bug#34100: 26.1; Incomplete ? and ?? handling in rx

bug#34100: 26.1; Incomplete ? and ?? handling in rx

[Mattias Engdegård]

In rx, the ? and ?? operators can be written verbatim as ? and ?? (space and ? character), or by using symbols whose leading character needs to be escaped, \? and \?? respectively. The names come from Olin Shivers's SRE, but ? is not a special character in Scheme syntax, hence the character syntax hack.

However, the symbols only partially work:

(rx (\? "x") (\?? "y"))       --> "x?y?"  ; expected "x?y??"
(rx (minimal-match (\? "x"))) --> "x??"   ; expected "x?"

While it could be argued that only the character-based syntax should be used, the fact is that the symbols are accepted and seem to work, just in a subtly broken way.

The documentation is also not clear on this point, and a programmer knowing the elisp syntax might very well assume that the symbols are the ones to use.

Suggested fix:

diff --git a/lisp/emacs-lisp/rx.el b/lisp/emacs-lisp/rx.el
index a39fe55c32..8b4551d0d3 100644
--- a/lisp/emacs-lisp/rx.el
+++ b/lisp/emacs-lisp/rx.el
@@ -733,8 +733,8 @@
 is non-nil."
   (rx-check form)
   (setq form (rx-trans-forms form))
-  (let ((suffix (cond ((memq (car form) '(* + ?\s)) "")
-		      ((memq (car form) '(*? +? ??)) "?")
+  (let ((suffix (cond ((memq (car form) '(* + \? ?\s)) "")
+		      ((memq (car form) '(*? +? \?? ??)) "?")
 		      (rx-greedy-flag "")
 		      (t "?")))
 	(op (cond ((memq (car form) '(* *? 0+ zero-or-more)) "*")

bug#34100: rx \? and \??: patch

[Mattias Engdegård]

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

Here is a patch with a test.

[-- Attachment #2: 0001-Make-the-rx-operators-and-behave-correctly.patch --]
[-- Type: application/octet-stream, Size: 2741 bytes --]

From b7ffa6c7220032ee1ea4d8773f58a37a647ecc22 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 16 Jan 2019 14:57:12 +0100
Subject: [PATCH] Make the rx operators \? and \?? behave correctly

* lisp/emacs-lisp/rx.el (rx-kleene):
Treat \? and \?? like ? and ?? (Bug#34100).
* test/lisp/emacs-lisp/rx-tests.el: Add tests for all repetition operators.
---
 lisp/emacs-lisp/rx.el            |  4 ++--
 test/lisp/emacs-lisp/rx-tests.el | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/rx.el b/lisp/emacs-lisp/rx.el
index a39fe55c32..8b4551d0d3 100644
--- a/lisp/emacs-lisp/rx.el
+++ b/lisp/emacs-lisp/rx.el
@@ -733,8 +733,8 @@ If OP is anything else, produce a greedy regexp if `rx-greedy-flag'
 is non-nil."
   (rx-check form)
   (setq form (rx-trans-forms form))
-  (let ((suffix (cond ((memq (car form) '(* + ?\s)) "")
-		      ((memq (car form) '(*? +? ??)) "?")
+  (let ((suffix (cond ((memq (car form) '(* + \? ?\s)) "")
+		      ((memq (car form) '(*? +? \?? ??)) "?")
 		      (rx-greedy-flag "")
 		      (t "?")))
 	(op (cond ((memq (car form) '(* *? 0+ zero-or-more)) "*")
diff --git a/test/lisp/emacs-lisp/rx-tests.el b/test/lisp/emacs-lisp/rx-tests.el
index 392a38ab95..f15e1016f7 100644
--- a/test/lisp/emacs-lisp/rx-tests.el
+++ b/test/lisp/emacs-lisp/rx-tests.el
@@ -65,5 +65,28 @@
                     (list u v)))
                  '("1" "3"))))
 
+(ert-deftest rx-kleene ()
+  "Test greedy and non-greedy repetition operators."
+  (should (equal (rx (* "a") (+ "b") (\? "c") (?\s "d")
+                     (*? "e") (+? "f") (\?? "g") (?? "h"))
+                 "a*b+c?d?e*?f+?g??h??"))
+  (should (equal (rx (zero-or-more "a") (0+ "b")
+                     (one-or-more "c") (1+ "d")
+                     (zero-or-one "e") (optional "f") (opt "g"))
+                 "a*b*c+d+e?f?g?"))
+  (should (equal (rx (minimal-match
+                      (seq (* "a") (+ "b") (\? "c") (?\s "d")
+                           (*? "e") (+? "f") (\?? "g") (?? "h"))))
+                 "a*b+c?d?e*?f+?g??h??"))
+  (should (equal (rx (minimal-match
+                      (seq (zero-or-more "a") (0+ "b")
+                           (one-or-more "c") (1+ "d")
+                           (zero-or-one "e") (optional "f") (opt "g"))))
+                 "a*?b*?c+?d+?e??f??g??"))
+  (should (equal (rx (maximal-match
+                      (seq (* "a") (+ "b") (\? "c") (?\s "d")
+                         (*? "e") (+? "f") (\?? "g") (?? "h"))))
+                 "a*b+c?d?e*?f+?g??h??")))
+
 (provide 'rx-tests)
 ;; rx-tests.el ends here.
-- 
2.17.2 (Apple Git-113)

bug#34100: Acknowledgement (26.1; Incomplete ? and ?? handling in rx)

[Mattias Engdegård]

Anything more I can do? I thought this one would be rather straightforward.
I do understand that there are more pressing matters.

bug#34100: Acknowledgement (26.1; Incomplete ? and ?? handling in rx)

[Michael Heerdegen]

Mattias Engdegård <mattiase@acm.org> writes:

> Anything more I can do? I thought this one would be rather
> straightforward.  I do understand that there are more pressing
> matters.

I guess you just have to be patient until someone has a look.  That can
sometimes take some time, there are a lot of open bug reports...

For me what you suggest looks like a good idea btw.

Michael.

bug#34100: Acknowledgement (26.1; Incomplete ? and ?? handling in rx)

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Mon, 28 Jan 2019 16:53:28 +0100
> Cc: 34100@debbugs.gnu.org
> 
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> > Anything more I can do? I thought this one would be rather
> > straightforward.  I do understand that there are more pressing
> > matters.
> 
> I guess you just have to be patient until someone has a look.  That can
> sometimes take some time, there are a lot of open bug reports...
> 
> For me what you suggest looks like a good idea btw.

Sorry for the long delay, Mattias.  I've now pushed your changes to
the master branch, and I'm marking this bug done.

Thanks.