bug#24998: Commit 'b5b997b95...' (Checkdoc: Don't require a space before an arg list) breaks defvar

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 631 bytes --]

Vladimir Lomov <lomov.vl@gmail.com> schrieb am Do., 29. Dez. 2016 um
05:29 Uhr:

> Hello,
>
> I found that the commit 'b5b997b95' breaks a valid defvar, see
> attached example file 'b.el'. If I run 'M-x checkdoc' in Emacs after
> that commit (I tried in Emacs compiled from commit 112460da70) it shows
> me error message (copy-pasted from *Message* buffer):
>
>   forward-sexp: Scan error: "Containing expression ends prematurely", 60,
> 61
>
> while before that commit (I tested in Emacs compiled from commit
> e5ef59b87d) it works like a charm.
>
>
Thanks for reporting. I've attached a patch that replaces the regex
completely.

[-- Attachment #1.2: Type: text/html, Size: 1194 bytes --]

[-- Attachment #2: 0001-Checkdoc-use-syntax-functions-instead-of-regex.txt --]
[-- Type: text/plain, Size: 6028 bytes --]

From 393cd12a5bd73aa2a0ef9f40e1c4db62b1219241 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 30 Dec 2016 18:00:54 +0100
Subject: [PATCH] Checkdoc: use syntax functions instead of regex

In checkdoc.el, get rid of the error-prone regex to find definition
forms, and use existing syntax-based navigation functions instead.
This fixes a corner case with one-argument `defvar' forms.

* lisp/emacs-lisp/checkdoc.el (checkdoc--next-docstring): New function.
(checkdoc-next-docstring, checkdoc-defun): Use it.
* test/lisp/emacs-lisp/checkdoc-tests.el (checkdoc-tests--next-docstring):
Add unit test.
---
 lisp/emacs-lisp/checkdoc.el            | 61 ++++++++++++++++++----------------
 test/lisp/emacs-lisp/checkdoc-tests.el | 13 ++++++++
 2 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 2c8bc020d3..084289c3e2 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -294,12 +294,6 @@ checkdoc-comment-style-functions
 (defvar checkdoc-diagnostic-buffer "*Style Warnings*"
   "Name of warning message buffer.")
 
-(defvar checkdoc-defun-regexp
-  "^(def\\(un\\|var\\|custom\\|macro\\|const\\|subst\\|advice\\)\
-\\s-+\\(\\(\\sw\\|\\s_\\)+\\)[ \t\n]*"
-  "Regular expression used to identify a defun.
-A search leaves the cursor in front of the parameter list.")
-
 (defcustom checkdoc-verb-check-experimental-flag t
   "Non-nil means to attempt to check the voice of the doc string.
 This check keys off some words which are commonly misused.  See the
@@ -938,13 +932,31 @@ checkdoc-continue
 (defun checkdoc-next-docstring ()
   "Move to the next doc string after point, and return t.
 Return nil if there are no more doc strings."
-  (if (not (re-search-forward checkdoc-defun-regexp nil t))
-      nil
-    ;; search drops us after the identifier.  The next sexp is either
-    ;; the argument list or the value of the variable.  skip it.
-    (forward-sexp 1)
-    (skip-chars-forward " \n\t")
-    t))
+  (let (found)
+    (while (and (not (setq found (checkdoc--next-docstring)))
+                (beginning-of-defun -1)))
+    found))
+
+(defun checkdoc--next-docstring ()
+  "When looking at a definition with a doc string, find it.
+Move to the next doc string after point, and return t.  When not
+looking at a definition containing a doc string, return nil and
+don't move point."
+  (pcase (save-excursion (condition-case nil
+                             (read (current-buffer))
+                           ;; Conservatively skip syntax errors.
+                           (invalid-read-syntax)))
+    (`(,(or 'defun 'defvar 'defcustom 'defmacro 'defconst 'defsubst 'defadvice)
+       ,(pred symbolp)
+       ;; Require an initializer, i.e. ignore single-argument `defvar'
+       ;; forms, which never have a doc string.
+       ,_ . ,_)
+     (down-list)
+     ;; Skip over function or macro name, symbol to be defined, and
+     ;; initializer or argument list.
+     (forward-sexp 3)
+     (skip-chars-forward " \n\t")
+     t)))
 
 ;;;###autoload
 (defun checkdoc-comments (&optional take-notes)
@@ -1020,28 +1032,19 @@ checkdoc-eval-defun
 ;;;###autoload
 (defun checkdoc-defun (&optional no-error)
   "Examine the doc string of the function or variable under point.
-Call `error' if the doc string has problems.  If NO-ERROR is
+Call `error' if the doc string has problems.  If O-ERROR is
 non-nil, then do not call error, but call `message' instead.
 If the doc string passes the test, then check the function for rogue white
 space at the end of each line."
   (interactive)
   (save-excursion
     (beginning-of-defun)
-    (if (not (looking-at checkdoc-defun-regexp))
-	;; I found this more annoying than useful.
-	;;(if (not no-error)
-	;;    (message "Cannot check this sexp's doc string."))
-	nil
-      ;; search drops us after the identifier.  The next sexp is either
-      ;; the argument list or the value of the variable.  skip it.
-      (goto-char (match-end 0))
-      (forward-sexp 1)
-      (skip-chars-forward " \n\t")
+    (when (checkdoc--next-docstring)
       (let* ((checkdoc-spellcheck-documentation-flag
-	      (car (memq checkdoc-spellcheck-documentation-flag
+              (car (memq checkdoc-spellcheck-documentation-flag
                          '(defun t))))
-	     (beg (save-excursion (beginning-of-defun) (point)))
-	     (end (save-excursion (end-of-defun) (point))))
+             (beg (save-excursion (beginning-of-defun) (point)))
+             (end (save-excursion (end-of-defun) (point))))
         (dolist (fun (list #'checkdoc-this-string-valid
                            (lambda () (checkdoc-message-text-search beg end))
                            (lambda () (checkdoc-rogue-space-check-engine beg end))))
@@ -1049,8 +1052,8 @@ checkdoc-defun
             (if msg (if no-error
                         (message "%s" (checkdoc-error-text msg))
                       (user-error "%s" (checkdoc-error-text msg))))))
-	(if (called-interactively-p 'interactive)
-	    (message "Checkdoc: done."))))))
+        (if (called-interactively-p 'interactive)
+            (message "Checkdoc: done."))))))
 
 ;;; Ispell interface for forcing a spell check
 ;;
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index 18b5a499e0..02db88c17e 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -37,4 +37,17 @@
     (insert "(defun foo())")
     (should-error (checkdoc-defun) :type 'user-error)))
 
+(ert-deftest checkdoc-tests--next-docstring ()
+  "Checks that the one-argument form of `defvar' works.
+See the comments in Bug#24998."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(defvar foo)
+\(defvar foo bar \"baz\")
+\(require 'foo)")
+    (goto-char (point-min))
+    (should (checkdoc-next-docstring))
+    (should (looking-at-p "\"baz\")"))
+    (should-not (checkdoc-next-docstring))))
+
 ;;; checkdoc-tests.el ends here
-- 
2.11.0