bug#16368: [PATCH] cperl-mode: don't freeze over a cool regexp

["Harald Jörg" <haj@posteo.de>]

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

On 9/3/20 11:58 AM, Stefan Kangas wrote:
> Harald Jörg <haj@posteo.de> writes:
>
>> I'd say this is desired behavior.
>
> OK, thanks.  Your explanation sounds good to me.

It wasn't quite accurate, though.  My explanation assumed that the
regex as a whole wasn't terminated, but in fact it was.  In fact, the
message comes from the fact that (?{...}) introduces a block of code
into the regexp.  So, by removing the colon from (?:{...}), the
semantics changed from "just another shy group" to "code", and for
code cperl-mode applies stricter rules than for the contents of a
capture group.

I'd still say the message is ok.

>> I can only guess that the first part of the message (which starts with
>> "cperl-forward-group-in-re") was added out of frustration: The bug
>> prevented the second part of the message (without
>> "cperl-forward-group-in-re") from ever appearing.  Only this wasn't
>> fatal unless... there was this closing brace two characters before.
>> I'll check that, and prepare an updated patch if that's true.
>
> Sounds good.

...And done.  Now you get only one message, without the unnecessary
"cperl-forward-group-in-re" prefix.

>> No problem, I'll do so.  I thought I was supposed to create the commit
>> messages with C-x 4 a, but probably I misunderstood and should have
>> post-processed that text in the first place.
>
> I always use C-x 4 a, and then delete the spacing to the left.

Ok, adapted.  I have also used your recommendation for the commit
summary.

>> Hm. That should rather be _moving_ that line to the top?
>
> Ah, right.  Yup, that sounds good.

When I did this, I stumbled over the purpose of Stefan Monniers change
to the tests - this has been taken to emacs-devel.  For now, moving
the line to the top and skipping the test if called in a perl-mode
environment, should do the trick.

Patch, mark2, is attached!
-- 
Cheers,
haj

[-- Attachment #2: 0001-Fix-freeze-in-cperl-mode-when-editing-a-regexp.patch --]
[-- Type: text/x-patch, Size: 3363 bytes --]

From 2ae8d341de4cb9782241348b28e4f713c317925c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Harald=20J=C3=B6rg?= <haj@posteo.de>
Date: Thu, 3 Sep 2020 22:11:47 +0200
Subject: [PATCH] Fix freeze in cperl-mode when editing a regexp

* lisp/progmodes/cperl-mode.el (cperl-forward-group-in-re): Make
sure that an error is reported back to the caller (Bug#16368).

* test/lisp/progmodes/cperl-mode-tests.el (cperl-mode-test-bug-16368):
Tests for balanced (no error) and unbalanced (caught exception)
cases of `cperl-forward-group-in-re'.
---
 lisp/progmodes/cperl-mode.el            |  9 ++++-----
 test/lisp/progmodes/cperl-mode-tests.el | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 44579cfd38..e2628c834c 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -3241,8 +3241,8 @@ cperl-forward-group-in-re
 Works before syntax recognition is done."
   ;; Works *before* syntax recognition is done
   (or st-l (setq st-l (list nil)))	; Avoid overwriting '()
-  (let (st b reset-st)
-    (condition-case b
+  (let (st result reset-st)
+    (condition-case err
 	(progn
 	  (setq st (cperl-cached-syntax-table st-l))
 	  (modify-syntax-entry ?\( "()" st)
@@ -3250,8 +3250,7 @@ cperl-forward-group-in-re
 	  (setq reset-st (syntax-table))
 	  (set-syntax-table st)
 	  (forward-sexp 1))
-      (error (message
-	      "cperl-forward-group-in-re: error %s" b)))
+      (error (setq result err)))
     ;; now restore the initial state
     (if st
 	(progn
@@ -3259,7 +3258,7 @@ cperl-forward-group-in-re
 	  (modify-syntax-entry ?\) "." st)))
     (if reset-st
 	(set-syntax-table reset-st))
-    b))
+    result))
 
 
 (defvar font-lock-string-face)
diff --git a/test/lisp/progmodes/cperl-mode-tests.el b/test/lisp/progmodes/cperl-mode-tests.el
index be8b42d99a..e14ff98e3f 100644
--- a/test/lisp/progmodes/cperl-mode-tests.el
+++ b/test/lisp/progmodes/cperl-mode-tests.el
@@ -18,6 +18,8 @@
 
 (defvar cperl-test-mode #'cperl-mode)
 
+(require 'cperl-mode)
+
 (defun cperl-test-ppss (text regexp)
   "Return the `syntax-ppss' of the first character matched by REGEXP in TEXT."
   (interactive)
@@ -48,4 +50,25 @@ cperl-mode-test-bug-42168
   (let ((code "{ $a- / $b } # /"))
     (should (equal (nth 8 (cperl-test-ppss code "/")) 7))))
 
+(ert-deftest cperl-mode-test-bug-16368 ()
+  "Verify that `cperl-forward-group-in-re' doesn't hide errors."
+  (skip-unless (eq cperl-test-mode #'cperl-mode))
+  (let ((code "/(\\d{4})(?{2}/;")     ; the regex from the bug report
+        (result))
+    (with-temp-buffer
+      (insert code)
+      (goto-char 9)
+      (setq result (cperl-forward-group-in-re))
+      (should (equal (car result) 'scan-error))
+      (should (equal (nth 1 result) "Unbalanced parentheses"))
+      (should (= (point) 9))))        ; point remains unchanged on error
+  (let ((code "/(\\d{4})(?{2})/;")    ; here all parens are balanced
+        (result))
+    (with-temp-buffer
+      (insert code)
+      (goto-char 9)
+      (setq result (cperl-forward-group-in-re))
+      (should (equal result nil))
+      (should (= (point) 15)))))      ; point has skipped the group
+
 ;;; cperl-mode-tests.el ends here
-- 
2.20.1