bug#26070: 26.0.50; js-mode slash insertion bug

bug#26070: 26.0.50; js-mode slash insertion bug

[Richard Copley]

After inserting a slash before other text in a js-mode buffer,
typing more characters has no effect, until you type C-g to quit.

Recipe to reproduce from 'emacs -Q':
M-x js-mode RET
x   ;; inserts "x"
C-b ;; backward char
/   ;; inserts "/"
x   ;; --- no effect! ---
C-g ;; quit

I couldn't see what code is running, because C-g doesn't enter the
debugger in this situation even when debug-on-quit is true, but I
bisected it to this commit:

commit 862d6438cfa6c6c035033697751f3d002357b024
Author: Tom Tromey <tom@tromey.com>
Date:   Sun Feb 5 11:40:18 2017 -0700

    Recognize JS regexp literals more correctly

In GNU Emacs 26.0.50 (build 6, x86_64-w64-mingw32)
 of 2017-03-12 built on MACHINE
Repository revision: 026c2cbf354fab138a65ad7093f17fbb23edb23c
Windowing system distributor 'Microsoft Corp.', version 10.0.14393
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --prefix=/mingw64 --with-modules
 --enable-locallisppath=/c/emacs/site-lisp 'CFLAGS=-O0 -g -ggdb''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND DBUS NOTIFY ACL GNUTLS LIBXML2 ZLIB
TOOLKIT_SCROLL_BARS MODULES

Important settings:
  value of $LANG: ENG
  locale-coding-system: cp1252

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-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
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message puny seq byte-opt subr-x gv
bytecomp byte-compile cl-extra help-mode cconv cl-loaddefs pcase cl-lib
dired dired-loaddefs format-spec rfc822 mml easymenu mml-sec
password-cache epa derived epg epg-config 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 time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp disp-table
term/w32-win w32-win w32-vars 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 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 w32notify dbusbind w32 multi-tty make-network-process emacs)

Memory information:
((conses 16 99056 6862)
 (symbols 56 20091 1)
 (miscs 48 48 87)
 (strings 32 19084 4910)
 (string-bytes 1 584358)
 (vectors 16 14743)
 (vector-slots 8 478865 4018)
 (floats 8 54 62)
 (intervals 56 230 0)
 (buffers 976 11))

bug#26070: 26.0.50; js-mode slash insertion bug

[Tom Tromey]

Richard> I couldn't see what code is running, because C-g doesn't enter the
Richard> debugger in this situation even when debug-on-quit is true, but I
Richard> bisected it to this commit:

Thanks for finding this and bisecting it.

At first I thought the problem was that the regexp literal matching code
in js-syntax-propertize should leave point after the construct.
However, the call to js-syntax-propertize really ought to do that... but
this code is always requiring the trailing "/", which is what is going
wrong.

I think once js-syntax-propertize is called, the regexp should always
succeed, so this patch makes the trailing "/" optional.

Can you try this and let me know if it works for you?

Tom

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 84c9111..fa865db 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1714,7 +1714,7 @@ js--syntax-propertize-regexp-regexp
               (not (any ?\] ?\\))
               (and "\\" not-newline)))
          "]")))
-   (group "/"))
+   (group (zero-or-one "/")))
   "Regular expression matching a JavaScript regexp literal.")
 
 (defun js-syntax-propertize-regexp (end)
@@ -1724,8 +1724,8 @@ js-syntax-propertize-regexp
       (goto-char (nth 8 ppss))
       (when (and (looking-at js--syntax-propertize-regexp-regexp)
                  ;; Don't touch text after END.
-                 (<= (match-end 1) end))
-        (put-text-property (match-beginning 1) (match-end 1)
+                 (or (not (match-end 1)) (<= (match-end 1) end)))
+        (put-text-property (match-beginning 1) (or (match-end 1) (match-end 0))
                            'syntax-table (string-to-syntax "\"/"))
         (goto-char (match-end 0))))))
 

bug#26070: 26.0.50; js-mode slash insertion bug

[Richard Copley]

> Can you try this and let me know if it works for you?

Thanks! I had a very quick play around and it seems to work.

bug#26070: 26.0.50; js-mode slash insertion bug

[Dmitry Gutov]

On 13.03.2017 15:50, Tom Tromey wrote:

> I think once js-syntax-propertize is called, the regexp should always
> succeed, so this patch makes the trailing "/" optional.

Does it solve the infloop in the case like:

/x[

as well?

If not, I think the following is a better patch (could use some 
tweaking, probably):

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index aed42a8..d9e6ef3 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1721,12 +1721,14 @@ js-syntax-propertize-regexp
      (when (eq (nth 3 ppss) ?/)
        ;; A /.../ regexp.
        (goto-char (nth 8 ppss))
-      (when (and (looking-at js--syntax-propertize-regexp-regexp)
-                 ;; Don't touch text after END.
-                 (<= (match-end 1) end))
-        (put-text-property (match-beginning 1) (match-end 1)
-                           'syntax-table (string-to-syntax "\"/"))
-        (goto-char (match-end 0))))))
+      (if (and (looking-at js--syntax-propertize-regexp-regexp)
+               ;; Don't touch text after END.
+               (<= (match-end 1) end))
+          (progn
+            (put-text-property (match-beginning 1) (match-end 1)
+                               'syntax-table (string-to-syntax "\"/"))
+            (goto-char (match-end 0)))
+        (goto-char (1+ (match-beginning 0)))))))

  (defun js-syntax-propertize (start end)
    ;; JavaScript allows immediate regular expression objects, written 
/.../.

bug#26070: 26.0.50; js-mode slash insertion bug

[Tom Tromey]

>>>>> "Dmitry" == Dmitry Gutov <dgutov@yandex.ru> writes:

>> I think once js-syntax-propertize is called, the regexp should always
>> succeed, so this patch makes the trailing "/" optional.

Dmitry> Does it solve the infloop in the case like:
Dmitry> /x[
Dmitry> as well?

Yes, with the patch I don't get an infloop no matter which way I insert
those characters.

Tom

bug#26070: 26.0.50; js-mode slash insertion bug

[Richard Copley]

On 14 March 2017 at 11:06, Tom Tromey <tom@tromey.com> wrote:
>>>>>> "Dmitry" == Dmitry Gutov <dgutov@yandex.ru> writes:
>
>>> I think once js-syntax-propertize is called, the regexp should always
>>> succeed, so this patch makes the trailing "/" optional.
>
> Dmitry> Does it solve the infloop in the case like:
> Dmitry> /x[
> Dmitry> as well?
>
> Yes, with the patch I don't get an infloop no matter which way I insert
> those characters.
>
> Tom

Caught one!

Insert the text below in a JS-mode buffer, go to the beginning of the
blank first line in function h() and type "/". There is an infloop.

The infloop doesn't happen if the "/" in function g() is changed to
a different operator.

You can replace the 0s with other stuff, but you need at least that
many characters (delete a 0 or a space and there is no infloop).

function f() {
    function g()
    {
        1 / 2;
    }

    function h() {

        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00000000000000000000000000000000000000000000000000;
        00;
    }
}

bug#26070: 26.0.50; js-mode slash insertion bug

[Tom Tromey]

>>>>> "Richard" == Richard Copley <rcopley@gmail.com> writes:

Richard> Caught one!

Thank you.

Richard> Insert the text below in a JS-mode buffer, go to the beginning of the
Richard> blank first line in function h() and type "/". There is an infloop.

How did you find this?  I'm very curious.

Anyway, please try this patch on top of the previous one I sent.
Or if you'd prefer I can send a combined patch.

Tom

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index fa865db..c220353 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1722,12 +1722,13 @@ js-syntax-propertize-regexp
     (when (eq (nth 3 ppss) ?/)
       ;; A /.../ regexp.
       (goto-char (nth 8 ppss))
-      (when (and (looking-at js--syntax-propertize-regexp-regexp)
-                 ;; Don't touch text after END.
-                 (or (not (match-end 1)) (<= (match-end 1) end)))
-        (put-text-property (match-beginning 1) (or (match-end 1) (match-end 0))
+      (when (looking-at js--syntax-propertize-regexp-regexp)
+        ;; Don't touch text after END.
+        (when (> end (match-end 1))
+          (setq end (match-end 1)))
+        (put-text-property (match-beginning 1) end
                            'syntax-table (string-to-syntax "\"/"))
-        (goto-char (match-end 0))))))
+        (goto-char end)))))
 
 (defun js-syntax-propertize (start end)
   ;; JavaScript allows immediate regular expression objects, written /.../.

bug#26070: 26.0.50; js-mode slash insertion bug

[Richard Copley]

On 22 March 2017 at 22:18, Tom Tromey <tom@tromey.com> wrote:
>>>>>> "Richard" == Richard Copley <rcopley@gmail.com> writes:
>
> Richard> Caught one!
>
> Thank you.
>
> Richard> Insert the text below in a JS-mode buffer, go to the beginning of the
> Richard> blank first line in function h() and type "/". There is an infloop.
>
> How did you find this?  I'm very curious.

I didn't go looking. The recipe is reduced from a program I was
writing. The infloop happened when I went to comment out a line.
Would have happened to somebody sooner or later :)

> Anyway, please try this patch on top of the previous one I sent.

Seems good, thanks very much.

> Or if you'd prefer I can send a combined patch.
>
> Tom
>
> diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
> index fa865db..c220353 100644
> --- a/lisp/progmodes/js.el
> +++ b/lisp/progmodes/js.el
> @@ -1722,12 +1722,13 @@ js-syntax-propertize-regexp
>      (when (eq (nth 3 ppss) ?/)
>        ;; A /.../ regexp.
>        (goto-char (nth 8 ppss))
> -      (when (and (looking-at js--syntax-propertize-regexp-regexp)
> -                 ;; Don't touch text after END.
> -                 (or (not (match-end 1)) (<= (match-end 1) end)))
> -        (put-text-property (match-beginning 1) (or (match-end 1) (match-end 0))
> +      (when (looking-at js--syntax-propertize-regexp-regexp)
> +        ;; Don't touch text after END.
> +        (when (> end (match-end 1))
> +          (setq end (match-end 1)))
> +        (put-text-property (match-beginning 1) end
>                             'syntax-table (string-to-syntax "\"/"))
> -        (goto-char (match-end 0))))))
> +        (goto-char end)))))
>
>  (defun js-syntax-propertize (start end)
>    ;; JavaScript allows immediate regular expression objects, written /.../.

bug#26070: 26.0.50; js-mode slash insertion bug

[Tom Tromey]

Richard> Seems good, thanks very much.

Thanks for giving it a try.

Here's the latest version of these two patches, with test cases.
Dmitry, please let me know what you think.

Tom

commit ff870ef0324a3e5c43cf2d183df5db72e824efca
Author: Tom Tromey <tom@tromey.com>
Date:   Mon Mar 13 21:53:59 2017 +0100

    fix two js-mode syntax propertization bugs
    
    Bug#26070:
    * lisp/progmodes/js.el (js--syntax-propertize-regexp-regexp): Add
    zero-or-one to regular expression.
    (js-syntax-propertize-regexp): Update.  Propertize body of regexp
    literal up to END.
    * test/lisp/progmodes/js-tests.el (js-mode-propertize-bug-1)
    (js-mode-propertize-bug-2): New tests.

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index aed42a8..3c720c0 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1713,7 +1713,7 @@ js--syntax-propertize-regexp-regexp
               (not (any ?\] ?\\))
               (and "\\" not-newline)))
          "]")))
-   (group "/"))
+   (group (zero-or-one "/")))
   "Regular expression matching a JavaScript regexp literal.")
 
 (defun js-syntax-propertize-regexp (end)
@@ -1721,12 +1721,13 @@ js-syntax-propertize-regexp
     (when (eq (nth 3 ppss) ?/)
       ;; A /.../ regexp.
       (goto-char (nth 8 ppss))
-      (when (and (looking-at js--syntax-propertize-regexp-regexp)
-                 ;; Don't touch text after END.
-                 (<= (match-end 1) end))
-        (put-text-property (match-beginning 1) (match-end 1)
+      (when (looking-at js--syntax-propertize-regexp-regexp)
+        ;; Don't touch text after END.
+        (when (> end (match-end 1))
+          (setq end (match-end 1)))
+        (put-text-property (match-beginning 1) end
                            'syntax-table (string-to-syntax "\"/"))
-        (goto-char (match-end 0))))))
+        (goto-char end)))))
 
 (defun js-syntax-propertize (start end)
   ;; JavaScript allows immediate regular expression objects, written /.../.
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index e030675..8e1bac1 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -140,6 +140,43 @@
       (font-lock-ensure)
       (should (eq (get-text-property (point) 'face) (caddr test))))))
 
+(ert-deftest js-mode-propertize-bug-1 ()
+  (with-temp-buffer
+    (js-mode)
+    (save-excursion (insert "x"))
+    (insert "/")
+    ;; The bug was a hang.
+    (should t)))
+
+(ert-deftest js-mode-propertize-bug-2 ()
+  (with-temp-buffer
+    (js-mode)
+    (insert "function f() {
+    function g()
+    {
+        1 / 2;
+    }
+
+    function h() {
+")
+    (save-excursion
+      (insert "
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00000000000000000000000000000000000000000000000000;
+        00;
+    }
+}
+"))
+    (insert "/")
+    ;; The bug was a hang.
+    (should t)))
+
 (provide 'js-tests)
 
 ;;; js-tests.el ends here

bug#26070: 26.0.50; js-mode slash insertion bug

[Dmitry Gutov]

On 24.03.2017 05:53, Tom Tromey wrote:

> Here's the latest version of these two patches, with test cases.
> Dmitry, please let me know what you think.

It's okay with me, thanks. As long as we're sure that 
js--syntax-propertize-regexp-regexp will always match.

bug#26070: fixed

[Tom Tromey]

I've checked in the patch.

Tom