regexp linting run in Emacs tree

regexp linting run in Emacs tree

[Mattias Engdegård]

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

Yet another scan of regexp mistakes in elisp files in the Emacs source tree, using relint 1.9.
Minor improvements to the regexp detection in relint has expanded the scope. There may be a few false positives.


[-- Attachment #2: relint.log --]
[-- Type: application/octet-stream, Size: 2463 bytes --]

;; -*- compilation -*-
lisp/calendar/diary-lib.el:112:42: In diary-glob-file-regexp-prefix: Escaped non-special character `#' (pos 1)
  "^\\#"
   .^
lisp/cedet/inversion.el:118:25: In call to string-match: Repetition of expression matching an empty string (pos 36)
  "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)?\\s-*\\.?alpha\\([0-9]+\\)?$"
   ............................................^
lisp/org/ob-haskell.el:180:33: In call to re-search-forward: Repetition of expression matching an empty string (pos 40)
  "^\\([ \t]*\\)#\\+begin_src[ \t]haskell*\\(.*\\)?[\r\n]\\([^\000]*?\\)[\r\n][ \t]*#\\+end_src.*"
   ...............................................^
lisp/org/org-capture.el:1688:37: In call to re-search-forward: Duplicated `a-z' inside character alternative (pos 9)
  "%\\(:[-a-za-z]+\\|<\\([^>\n]+\\)>\\|[aAcfFikKlntTuUx]\\)"
   ..........^
lisp/progmodes/cc-mode.el:1400:26: In call to re-search-forward: Branch matches subset of a previous branch (pos 12)
  "\\(\\\\\\(.\\|\n\\|\r\\)\\|[^\\\n\r]\\)*"
   ...................^
lisp/progmodes/cc-mode.el:1508:33: In call to re-search-forward: Branch matches subset of a previous branch (pos 12)
  "\\(\\\\\\(.\\|\n\\|\r\\)\\|[^\\\n\r]\\)*"
   ...................^
lisp/progmodes/fortran.el:1823:36: In call to skip-chars-backward: Duplicated character ` ' (pos 2)
  " \t "
   ...^
lisp/progmodes/fortran.el:1824:65: In call to skip-chars-forward: Duplicated character ` ' (pos 2)
  " \t "
   ...^
lisp/progmodes/gdb-mi.el:2718:33: In call to re-search-forward: Literal `-' not first or last in character alternative (pos 12)
  "\\([[:alnum:]-_]+\\)="
   .............^
lisp/isearch.el:2191:23: In call to string-match-p: Error: Unknown syntax code ‘@’: "\\`\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+\\'"
lisp/isearch.el:2193:30: In call to string-match-p: Error: Unknown syntax code ‘@’: "\\`\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+"
lisp/isearch.el:2195:56: In call to split-string: Error: Unknown syntax code ‘@’: "\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+"
lisp/isearch.el:2196:30: In call to string-match-p: Error: Unknown syntax code ‘@’: "\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+\\'"

Re: regexp linting run in Emacs tree

[Paul Eggert]

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

Thanks, I installed the attached to master.

The following seem to be false alarms, though:

lisp/progmodes/fortran.el:1823:36: In call to skip-chars-backward: Duplicated 
character ` ' (pos 2)
   " \t "
    ...^
[and similar matches] This seem to be assuming that the customized variable 
fortran-comment-indent-char has its default value.

lisp/isearch.el:2191:23: In call to string-match-p: Error: Unknown syntax code 
‘@’: 
"\\`\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+\\'"
[and similar matches] This seems to be assuming that \s@ is not valid, but that 
escape is documented as valid. Is there some problem with \s@ that I don't know 
about?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-2019-08-04-regex-lint.patch --]
[-- Type: text/x-patch; name="0001-Fix-2019-08-04-regex-lint.patch", Size: 5713 bytes --]

From e47cb08df1d90e1d22ec3230ef465c23c3c1f8e4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 4 Aug 2019 11:39:03 -0700
Subject: [PATCH] Fix 2019-08-04 regex lint
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Mattias Engdegård in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00085.html
* lisp/calendar/diary-lib.el (diary-glob-file-regexp-prefix):
Omit unnecessary ‘\’ before ordinary char.
* lisp/cedet/inversion.el (inversion-decoders):
* lisp/org/ob-haskell.el (org-babel-haskell-export-to-lhs):
Omit unnecessary ‘?’ after nullable pattern.
* lisp/org/org-capture.el (org-capture-fill-template):
Match upper-case as well as lower-case letters.
* lisp/progmodes/cc-mode.el (c-before-change-check-unbalanced-strings)
(c-after-change-mark-abnormal-strings):
Simplify ‘.|\r’ to ‘.’.
* lisp/progmodes/gdb-mi.el (gdb-jsonify-buffer):
Put ‘-’ at end of bracket expression.
---
 lisp/calendar/diary-lib.el | 2 +-
 lisp/cedet/inversion.el    | 6 +++---
 lisp/org/ob-haskell.el     | 2 +-
 lisp/org/org-capture.el    | 2 +-
 lisp/progmodes/cc-mode.el  | 4 ++--
 lisp/progmodes/gdb-mi.el   | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lisp/calendar/diary-lib.el b/lisp/calendar/diary-lib.el
index a0e90c439b..06f1161b44 100644
--- a/lisp/calendar/diary-lib.el
+++ b/lisp/calendar/diary-lib.el
@@ -109,7 +109,7 @@ diary-face-attrs
                                       :tag "A string, t, or nil"))))
   :group 'diary)
 
-(defcustom diary-glob-file-regexp-prefix "^\\#"
+(defcustom diary-glob-file-regexp-prefix "^#"
   "Regular expression pre-pended to `diary-face-attrs' for file-wide specifiers."
   :type 'regexp
   :group 'diary)
diff --git a/lisp/cedet/inversion.el b/lisp/cedet/inversion.el
index c62a57ee48..3bed9d7053 100644
--- a/lisp/cedet/inversion.el
+++ b/lisp/cedet/inversion.el
@@ -79,9 +79,9 @@ inversion-incompatible-version
 
 (defconst inversion-decoders
   '(
-    (alpha  "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)?\\s-*\\.?alpha\\([0-9]+\\)?$" 4)
-    (beta   "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)?\\s-*\\.?beta\\([0-9]+\\)?$" 4)
-    (beta   "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)?\\s-*\\.?(beta\\([0-9]+\\)?)$" 4)
+    (alpha  "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)\\s-*\\.?alpha\\([0-9]+\\)?$" 4)
+    (beta   "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)\\s-*\\.?beta\\([0-9]+\\)?$" 4)
+    (beta   "^\\([0-9]+\\)\\.\\([0-9]+\\)\\.?\\([0-9]*\\)\\s-*\\.?(beta\\([0-9]+\\)?)$" 4)
     (beta  "^[^/]+/\\w+--\\w+--\\([0-9]+\\)\\.\\([0-9]+\\)\\.\\([0-9]+\\)--patch-\\([0-9]+\\)" 4)
     (beta "^\\w+: v\\([0-9]+\\)\\.\\([0-9]+\\)\\.\\([0-9]+\\)-\\([0-9]+\\)-\\(.*\\)" 5)
     (prerelease "^\\([0-9]+\\)\\.\\([0-9]+\\)\\s-*\\.?pre\\([0-9]+\\)?$" 3)
diff --git a/lisp/org/ob-haskell.el b/lisp/org/ob-haskell.el
index 3c0a102fb2..50d1b57969 100644
--- a/lisp/org/ob-haskell.el
+++ b/lisp/org/ob-haskell.el
@@ -160,7 +160,7 @@ org-babel-haskell-export-to-lhs
   (interactive "P")
   (let* ((contents (buffer-string))
          (haskell-regexp
-          (concat "^\\([ \t]*\\)#\\+begin_src[ \t]haskell*\\(.*\\)?[\r\n]"
+          (concat "^\\([ \t]*\\)#\\+begin_src[ \t]haskell*\\(.*\\)[\r\n]"
                   "\\([^\000]*?\\)[\r\n][ \t]*#\\+end_src.*"))
          (base-name (file-name-sans-extension (buffer-file-name)))
          (tmp-file (org-babel-temp-file "haskell-"))
diff --git a/lisp/org/org-capture.el b/lisp/org/org-capture.el
index cbcf6c72f9..829872c382 100644
--- a/lisp/org/org-capture.el
+++ b/lisp/org/org-capture.el
@@ -1683,7 +1683,7 @@ org-capture-fill-template
       (org-capture-expand-embedded-elisp 'mark)
 
       ;; Expand non-interactive templates.
-      (let ((regexp "%\\(:[-a-za-z]+\\|<\\([^>\n]+\\)>\\|[aAcfFikKlntTuUx]\\)"))
+      (let ((regexp "%\\(:[-A-Za-z]+\\|<\\([^>\n]+\\)>\\|[aAcfFikKlntTuUx]\\)"))
 	(save-excursion
 	  (while (re-search-forward regexp nil t)
 	    ;; `org-capture-escaped-%' may modify buffer and cripple
diff --git a/lisp/progmodes/cc-mode.el b/lisp/progmodes/cc-mode.el
index 60a9de5ddb..76f5de212f 100644
--- a/lisp/progmodes/cc-mode.el
+++ b/lisp/progmodes/cc-mode.el
@@ -1397,7 +1397,7 @@ c-before-change-check-unbalanced-strings
 
       ;; Move to end of logical line (as it will be after the change, or as it
       ;; was before unescaping a NL.)
-      (re-search-forward "\\(\\\\\\(.\\|\n\\|\r\\)\\|[^\\\n\r]\\)*" nil t)
+      (re-search-forward "\\(\\\\\\(.\\|\n\\)\\|[^\\\n\r]\\)*" nil t)
       ;; We're at an EOLL or point-max.
       (if (equal (c-get-char-property (point) 'syntax-table) '(15))
 	  (if (memq (char-after) '(?\n ?\r))
@@ -1505,7 +1505,7 @@ c-after-change-mark-abnormal-strings
 	   (progn
 	     (goto-char (min (1+ end)	; 1+, in case a NL has become escaped.
 			     (point-max)))
-	     (re-search-forward "\\(\\\\\\(.\\|\n\\|\r\\)\\|[^\\\n\r]\\)*"
+	     (re-search-forward "\\(\\\\\\(.\\|\n\\)\\|[^\\\n\r]\\)*"
 				nil t)
 	     (point))
 	   c-new-END))
diff --git a/lisp/progmodes/gdb-mi.el b/lisp/progmodes/gdb-mi.el
index 439e0dfc62..48c7dde9f5 100644
--- a/lisp/progmodes/gdb-mi.el
+++ b/lisp/progmodes/gdb-mi.el
@@ -2714,7 +2714,7 @@ gdb-jsonify-buffer
               (insert "]"))))))
     (goto-char (point-min))
     (insert "{")
-    (let ((re (concat "\\([[:alnum:]-_]+\\)=")))
+    (let ((re (concat "\\([[:alnum:]_-]+\\)=")))
       (while (re-search-forward re nil t)
         (replace-match "\"\\1\":" nil nil)
         (if (eq (char-after) ?\") (forward-sexp) (forward-char))))
-- 
2.17.1

Re: regexp linting run in Emacs tree

[Mattias Engdegård]

4 aug. 2019 kl. 20.46 skrev Paul Eggert <eggert@cs.ucla.edu>:
> 
> Thanks, I installed the attached to master.

Thank you, this looks fine.

> The following seem to be false alarms, though:
> 
> lisp/progmodes/fortran.el:1823:36: In call to skip-chars-backward: Duplicated character ` ' (pos 2)
>  " \t "
>   ...^
> [and similar matches] This seem to be assuming that the customized variable fortran-comment-indent-char has its default value.

Yes, it is a bit hard to guard against that sort of thing --- relint blindly assumes default values of global variables. What about adding a `delete-dups' to make it shut up?

> lisp/isearch.el:2191:23: In call to string-match-p: Error: Unknown syntax code ‘@’: "\\`\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+\\'"
> [and similar matches] This seems to be assuming that \s@ is not valid, but that escape is documented as valid. Is there some problem with \s@ that I don't know about?

I assumed that @ isn't a valid syntax code in regexps, since it only means 'inherit from standard syntax table' which is fine for specifying the syntax of a character but makes no sense when we are trying to match something. Do you agree? (The regexp engine will accept it, but then again it will accept any character without complaining, which is probably a bug.)

Re: regexp linting run in Emacs tree

[Paul Eggert]

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

Mattias Engdegård wrote:

> relint blindly assumes default values of global variables. What about adding a `delete-dups' to make it shut up?

That would slow down and complicate the source a bit. Maybe just add the glitch 
to the delinter's exception list, if any.
> @ isn't a valid syntax code in regexps, since it only means 'inherit from standard syntax table' which is fine for specifying the syntax of a character but makes no sense when we are trying to match something.

Oh, thanks, I see the point now. I installed the attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-one-more-2019-08-04-regex-lint.patch --]
[-- Type: text/x-patch; name="0001-Fix-one-more-2019-08-04-regex-lint.patch", Size: 2139 bytes --]

From 00e6e5205a3eee2f965f5649fc3f98dd37c94437 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 4 Aug 2019 15:54:17 -0700
Subject: [PATCH] Fix one more 2019-08-04 regex lint
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem clarified by Mattias Engdegård in:
https://lists.gnu.org/r/emacs-devel/2019-08/msg00087.html
* lisp/isearch.el (isearch-symbol-regexp):
Remove \s@ from regexp as it cannot match.
---
 lisp/isearch.el | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 97c75b2978..09729034d7 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2184,16 +2184,19 @@ isearch-symbol-regexp
 the beginning or the end of the string need not match a symbol boundary."
   (let ((not-word-symbol-re
 	 ;; This regexp matches all syntaxes except word and symbol syntax.
-	 ;; FIXME: Replace it with something shorter if possible (bug#14602).
-	 "\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s@\\|\\s!\\|\\s|\\)+"))
+	 "\\(?:\\s-\\|\\s.\\|\\s(\\|\\s)\\|\\s\"\\|\\s\\\\|\\s/\\|\\s$\\|\\s'\\|\\s<\\|\\s>\\|\\s!\\|\\s|\\)+"))
     (cond
      ((equal string "") "")
-     ((string-match-p (format "\\`%s\\'" not-word-symbol-re) string) not-word-symbol-re)
+     ((string-match-p (format "\\`%s\\'" not-word-symbol-re) string)
+      not-word-symbol-re)
      (t (concat
-	 (if (string-match-p (format "\\`%s" not-word-symbol-re) string) not-word-symbol-re
+	 (if (string-match-p (format "\\`%s" not-word-symbol-re) string)
+	     not-word-symbol-re
 	   "\\_<")
-	 (mapconcat 'regexp-quote (split-string string not-word-symbol-re t) not-word-symbol-re)
-	 (if (string-match-p (format "%s\\'" not-word-symbol-re) string) not-word-symbol-re
+	 (mapconcat 'regexp-quote (split-string string not-word-symbol-re t)
+		    not-word-symbol-re)
+	 (if (string-match-p (format "%s\\'" not-word-symbol-re) string)
+	     not-word-symbol-re
 	   (unless lax "\\_>")))))))
 
 ;; Search with lax whitespace
-- 
2.17.1

Re: regexp linting run in Emacs tree

[Mattias Engdegård]

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

5 aug. 2019 kl. 00.57 skrev Paul Eggert <eggert@cs.ucla.edu>:
> 
>> relint blindly assumes default values of global variables. What about adding a `delete-dups' to make it shut up?
> 
> That would slow down and complicate the source a bit. Maybe just add the glitch to the delinter's exception list, if any.

There isn't one and I had hoped not having to add such a mechanism. Partly because it is a cop-out, and partly because it, too, would litter the source with alien comments like

  ;; relint: skip-set-duplicate

the kind of which, in my experience, tend to linger far beyond their usefulness and mystify innocent readers of the code.
(Sticking the suppressions in a separate .relint-exceptions file is not a better solution.)

What about the fairly lightweight attached patch? It's about as cheap as can be, while keeping it readable.

> Oh, thanks, I see the point now. I installed the attached.

Good, thank you. I especially like how you removed the "FIXME: replace it with something shorter" comment, since you did, in effect, do just that!


[-- Attachment #2: 0001-Shut-up-regexp-linter.patch --]
[-- Type: application/octet-stream, Size: 1283 bytes --]

From 2c648f73207813787175776e76fba426d67cb9f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 5 Aug 2019 12:59:53 +0200
Subject: [PATCH] Shut up regexp linter

* lisp/progmodes/fortran.el (fortran-indent-to-column):
Prevent relint from complaining about a duplicated character in the
argument to skip-chars-{forward,backward}.
---
 lisp/progmodes/fortran.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/fortran.el b/lisp/progmodes/fortran.el
index f01e866f55..8f58c501e4 100644
--- a/lisp/progmodes/fortran.el
+++ b/lisp/progmodes/fortran.el
@@ -1818,7 +1818,9 @@ fortran-indent-to-column
             (let* ((char (if (stringp fortran-comment-indent-char)
                              (aref fortran-comment-indent-char 0)
                            fortran-comment-indent-char))
-                   (chars (string ?\s ?\t char)))
+                   (chars (if (memq char '(?\s ?\t))
+                              " \t"
+                            (string ?\s ?\t char))))
               (goto-char (match-end 0))
               (skip-chars-backward chars)
               (delete-region (point) (progn (skip-chars-forward chars)
-- 
2.20.1 (Apple Git-117)

Re: regexp linting run in Emacs tree

[Paul Eggert]

Mattias Engdegård wrote:
> What about the fairly lightweight attached patch? It's about as cheap as can be, while keeping it readable.

I'm afraid that'd still be more clutter than it's worth, at least to me.

Instead, how about changing the delinter to omit this particular warning if the 
regexp was computed from a configurable variable (as opposed to an ordinary 
variable)?

Re: regexp linting run in Emacs tree

[Mattias Engdegård]

5 aug. 2019 kl. 19.17 skrev Paul Eggert <eggert@cs.ucla.edu>:
> 
> I'm afraid that'd still be more clutter than it's worth, at least to me.

Well, it's faster than the current code in the common case[*]. It also reflects the intended semantics better; the current code relies on the implicit tolerance for duplicates in skip-chars-forward.

> Instead, how about changing the delinter to omit this particular warning if the regexp was computed from a configurable variable (as opposed to an ordinary variable)?

That's a very ad-hoc rule which just happens to fit this very situation but isn't really causally connected to the false positive in a reasonable way. It could just as well have occurred in a different way, or resulted in a different warning. It just goes to show how hard it is to make an algorithm understand when a human programmer knowingly breaks the rules to achieve a certain effect.

As I see it, it's either the proposed memq change which makes the code faster and (arguably) clearer, a formal suppressive comment, or seeing this instance every time (and remembering to ignore it).

[*] Benchmarked:

(defun f (c) (string c ?\s ?\t))
(defun g (c) (if (memq c '(?\s ?\t)) " \t" (string c ?\s ?\t)))

g is about twice as fast as f for c=32; less allocation, and memq is a bytecode op while string isn't.

Re: regexp linting run in Emacs tree

[Mattias Engdegård]

5 aug. 2019 kl. 00.57 skrev Paul Eggert <eggert@cs.ucla.edu>:
> 
> That would slow down and complicate the source a bit. Maybe just add the glitch to the delinter's exception list, if any.

There is now a suppression mechanism in relint, so I added suppressions to fortran.el.

Relint now also checks arguments to `skip-syntax-forward' and `skip-syntax-backward', which caught something:

lisp/wid-edit.el:3331:34: In call to skip-syntax-forward: Invalid char `s' in syntax string (pos 1)
  "\\s-"
   ..^
lisp/wid-edit.el:3337:34: In call to skip-syntax-forward: Invalid char `s' in syntax string (pos 1)
  "\\s-"
   ..^

There are a few hits in the GNU ELPA repository as well.