Re: Forbid reverse ranges in rx

["Mattias Engdegård" <mattiase@acm.org>]

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

tis 2019-03-12 klockan 12:56 -0400 skrev Stefan Monnier:

> Indeed, I don't think we need to explain in detail, but I think it's
> likely that the naive user will simply not understand "reverse
> range".
> 
> Maybe instead of saying it's not permitted, we should say "range %c-
> %c
> is reversed"?

Right. I've followed your suggestion, and per Eli's advice moved the
NEWS item to 'Incompatible Changes'.


[-- Attachment #2: 0001-Disallow-reversed-char-ranges-in-rx.patch --]
[-- Type: text/x-patch, Size: 3646 bytes --]

From 8f85f3b5ce1ea9241e857c6b25860bb07fe3de11 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 12 Mar 2019 14:39:47 +0100
Subject: [PATCH] Disallow reversed char ranges in `rx'

(any "a-Z0-9") generated "[0-9]", and (any (?9 . ?0)) generated "[9-0]".
Reversed ranges are either mistakes or abuse.  Neither should be allowed.

etc/NEWS: Explain the change.
lisp/emacs-lisp/rx.el (rx): Document.
(rx-check-any-string, rx-check-any): Add error checks for reversed ranges.
test/lisp/emacs-lisp/rx-tests.el (rx-char-any-range-bad): New test.
---
 etc/NEWS                         |  7 +++++++
 lisp/emacs-lisp/rx.el            | 11 +++++++++--
 test/lisp/emacs-lisp/rx-tests.el |  4 ++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 410c1821ae..071854cf86 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1318,6 +1318,13 @@ they are now allocated like any other pseudovector.  As a result, the
 'misc' component, and the 'misc-objects-consed' variable has been
 removed.
 
++++
+** Reversed character ranges are no longer permitted in rx.
+Previously, ranges where the starting character is greater than the
+ending character were silently omitted.
+For example, '(rx (any "@z-a" (?9 . ?0)))' would match '@' only.
+Now, such rx expressions generate an error.
+
 \f
 * Lisp Changes in Emacs 27.1
 
diff --git a/lisp/emacs-lisp/rx.el b/lisp/emacs-lisp/rx.el
index f6deb45d44..fdd24317c6 100644
--- a/lisp/emacs-lisp/rx.el
+++ b/lisp/emacs-lisp/rx.el
@@ -482,7 +482,10 @@ rx-check-any-string
              (let ((start (funcall decode-char (aref str i)))
                    (end   (funcall decode-char (aref str (+ i 2)))))
                (cond ((< start end) (push (cons start end) ret))
-                     ((= start end) (push start ret)))
+                     ((= start end) (push start ret))
+                     (t
+                      (error "Rx character range `%c-%c' is reversed"
+                             start end)))
                (setq i (+ i 3))))
             (t
              ;; Single character.
@@ -503,7 +506,10 @@ rx-check-any
 	       (null (string-match "\\`\\[\\[:[-a-z]+:\\]\\]\\'" translation)))
 	   (error "Invalid char class `%s' in Rx `any'" arg))
        (list (substring translation 1 -1)))) ; strip outer brackets
-    ((and (integerp (car-safe arg)) (integerp (cdr-safe arg)))
+    ((and (characterp (car-safe arg)) (characterp (cdr-safe arg)))
+     (unless (<= (car arg) (cdr arg))
+       (error "Rx character range `%c-%c' is reversed"
+              (car arg) (cdr arg)))
      (list arg))
     ((stringp arg) (rx-check-any-string arg))
     ((error
@@ -916,6 +922,7 @@ rx
      matches any character in SET ....  SET may be a character or string.
      Ranges of characters can be specified as `A-Z' in strings.
      Ranges may also be specified as conses like `(?A . ?Z)'.
+     Reversed ranges like `Z-A' and `(?Z . ?A)' are not permitted.
 
      SET may also be the name of a character class: `digit',
      `control', `hex-digit', `blank', `graph', `print', `alnum',
diff --git a/test/lisp/emacs-lisp/rx-tests.el b/test/lisp/emacs-lisp/rx-tests.el
index fa3d9b0d5e..7410012b1f 100644
--- a/test/lisp/emacs-lisp/rx-tests.el
+++ b/test/lisp/emacs-lisp/rx-tests.el
@@ -40,6 +40,10 @@
   (should (equal (rx (any "\a-\n"))
                  "[\a-\n]")))
 
+(ert-deftest rx-char-any-range-bad ()
+  (should-error (rx (any "0-9a-Z")))
+  (should-error (rx (any (?0 . ?9) (?a . ?Z)))))
+
 (ert-deftest rx-char-any-raw-byte ()
   "Test raw bytes in character alternatives."
   ;; Separate raw characters.
-- 
2.20.1