bug#24998: 25.1.50; Checkdoc ignores functions if there is no space before the argument list

bug#24998: 25.1.50; Checkdoc ignores functions if there is no space before the argument list

[Philipp Stephani]

Create a small Elisp file such as (a.el):

;;; a.el --- aaa -*- lexical-binding: t; -*-

;;; Commentary:

;;; Code:

(defun a--bar () 123)

;;; a.el ends here

Run M-x checkdoc.  It will complain about the missing docstring for
`a--bar'.

Now remove the space before () and run checkdoc again.  It won't
complain any more.




In GNU Emacs 25.1.50.17 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2016-11-22 built on localhost
Repository revision: d4e15492f7c55eabdadc6f3735b84972748e6c0d
Windowing system distributor 'The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04 LTS

Configured using:
 'configure --with-modules --enable-checking
 --enable-check-lisp-object-type 'CFLAGS=-ggdb3 -O0''

Configured features:
XPM JPEG TIFF GIF PNG SOUND GSETTINGS NOTIFY GNUTLS FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent messages:
Wrote /tmp/a.el
Searching for doc string error...78%
All variables and subroutines might as well have a documentation string (C-h,e,n,p,q) [3 times]
When you’re done editing press C-M-c to continue.
Quit
Undo! [2 times]
C-M-x is undefined
Searching for doc string error...78%
All variables and subroutines might as well have a documentation string (C-h,e,n,p,q) [4 times]
Checkdoc: Done. [2 times]
Quit

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util help-fns mail-prsvr mail-utils
noutline outline easy-mmode lisp-mnt checkdoc thingatpt help-mode
easymenu cl-loaddefs pcase cl-lib time-date mule-util tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese charscript case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
inotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 93366 6706)
 (symbols 48 20314 0)
 (miscs 40 371 275)
 (strings 32 16393 4687)
 (string-bytes 1 481662)
 (vectors 16 13334)
 (vector-slots 8 452518 5000)
 (floats 8 176 129)
 (intervals 56 304 10)
 (buffers 976 25)
 (heap 1024 38041 1028))

-- 
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

Diese E-Mail ist vertraulich.  Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen
Sie die E-Mail und alle Anhänge.  Vielen Dank.

This e-mail is confidential.  If you are not the right addressee please do not
forward it, please inform the sender, and please erase this e-mail including
any attachments.  Thanks.

bug#24998: 25.1.50; Checkdoc ignores functions if there is no space before the argument list

[Philipp Stephani]

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

Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 23. Nov. 2016 um
11:11 Uhr:

>
> Create a small Elisp file such as (a.el):
>
> ;;; a.el --- aaa -*- lexical-binding: t; -*-
>
> ;;; Commentary:
>
> ;;; Code:
>
> (defun a--bar () 123)
>
> ;;; a.el ends here
>
> Run M-x checkdoc.  It will complain about the missing docstring for
> `a--bar'.
>
> Now remove the space before () and run checkdoc again.  It won't
> complain any more.
>

Attached a patch.

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

[-- Attachment #2: 0001-Checkdoc-Don-t-require-a-space-before-an-arg-list.txt --]
[-- Type: text/plain, Size: 2658 bytes --]

From 0a963d8e9053670c6f93803dc229adeb2d174081 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Wed, 23 Nov 2016 20:29:36 +0100
Subject: [PATCH] Checkdoc: Don't require a space before an arg list

See Bug#24998.

* lisp/emacs-lisp/checkdoc.el (checkdoc-defun-regexp): Don't require a
space before a argument list.
* test/lisp/emacs-lisp/checkdoc-tests.el (checkdoc-tests--bug-24998):
Add unit test.
---
 lisp/emacs-lisp/checkdoc.el            |  2 +-
 test/lisp/emacs-lisp/checkdoc-tests.el | 40 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 test/lisp/emacs-lisp/checkdoc-tests.el

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 769c2fe..2c8bc02 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -296,7 +296,7 @@ checkdoc-diagnostic-buffer
 
 (defvar checkdoc-defun-regexp
   "^(def\\(un\\|var\\|custom\\|macro\\|const\\|subst\\|advice\\)\
-\\s-+\\(\\(\\sw\\|\\s_\\)+\\)[ \t\n]+"
+\\s-+\\(\\(\\sw\\|\\s_\\)+\\)[ \t\n]*"
   "Regular expression used to identify a defun.
 A search leaves the cursor in front of the parameter list.")
 
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
new file mode 100644
index 0000000..18b5a49
--- /dev/null
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -0,0 +1,40 @@
+;;; checkdoc-tests.el --- unit tests for checkdoc.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2016  Free Software Foundation, Inc.
+
+;; Author: Google Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Unit tests for lisp/emacs-lisp/checkdoc.el.
+
+;;; Code:
+
+(require 'checkdoc)
+
+(require 'elisp-mode)
+(require 'ert)
+
+(ert-deftest checkdoc-tests--bug-24998 ()
+  "Checks that Bug#24998 is fixed."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(defun foo())")
+    (should-error (checkdoc-defun) :type 'user-error)))
+
+;;; checkdoc-tests.el ends here
-- 
2.8.0.rc3.226.g39d4020

bug#24998: 25.1.50; Checkdoc ignores functions if there is no space before the argument list

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 23 Nov 2016 19:35:52 +0000
> 
> Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 23. Nov. 2016 um 11:11 Uhr:
> 
>  Create a small Elisp file such as (a.el):
> 
>  ;;; a.el --- aaa -*- lexical-binding: t; -*-
> 
>  ;;; Commentary:
> 
>  ;;; Code:
> 
>  (defun a--bar () 123)
> 
>  ;;; a.el ends here
> 
>  Run M-x checkdoc. It will complain about the missing docstring for
>  `a--bar'.
> 
>  Now remove the space before () and run checkdoc again. It won't
>  complain any more.
> 
> Attached a patch.

Thanks, please push to master.

bug#24998: 25.1.50; Checkdoc ignores functions if there is no space before the argument list

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 2. Dez. 2016 um 10:02 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Wed, 23 Nov 2016 19:35:52 +0000
> >
> > Philipp Stephani <p.stephani2@gmail.com> schrieb am Mi., 23. Nov. 2016
> um 11:11 Uhr:
> >
> >  Create a small Elisp file such as (a.el):
> >
> >  ;;; a.el --- aaa -*- lexical-binding: t; -*-
> >
> >  ;;; Commentary:
> >
> >  ;;; Code:
> >
> >  (defun a--bar () 123)
> >
> >  ;;; a.el ends here
> >
> >  Run M-x checkdoc. It will complain about the missing docstring for
> >  `a--bar'.
> >
> >  Now remove the space before () and run checkdoc again. It won't
> >  complain any more.
> >
> > Attached a patch.
>
> Thanks, please push to master.
>

Pushed as 65b997b95e284e2edc1266663e39791f68d76ad7.

[-- Attachment #2: Type: text/html, Size: 1849 bytes --]

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

[Vladimir Lomov]

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

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.

---
WBR, Vladimir Lomov

-- 
"An anthropologist at Tulane has just come back from a field trip to New 
Guinea with reports of a tribe so primitive that they have Tide but not 
new Tide with lemon-fresh Borax."
-- David Letterman

[-- Attachment #2: b.el --]
[-- Type: text/plain, Size: 81 bytes --]

;;; b.el --- bbb

;;; Commentary:

;;; Code:

(defvar b-b-b)

;;; b.el ends here

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

[Vladimir Lomov]

** Vladimir Lomov [2016-12-29 12:28:06 +0800]:

> Hello,
> 
> I found that the commit 'b5b997b95' breaks a valid defvar,
[...]

on second thought, the original 'checkdoc-defun-regexp' seems to be
wrong. According to it's docstring, it's purpose to stop before
parameter list, but 'defvar' may not have that list.

---
WBR, Vladimir Lomov

-- 
<|Rain|> Knghtbrd: let me give you access to the zone files
<Knghtbrd> oh gods - you do realize I have never played with bind right?
<|Rain|> uhoh :)

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

[Philipp Stephani]

[-- 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

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

[Vladimir Lomov]

** Philipp Stephani [2016-12-30 17:04:04 +0000]:

> 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.

Thank you, your patch works fine on my example and other real el file.

---
WBR, Vladimir Lomov

-- 
God must have loved calories, she made so many of them.

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

[Philipp Stephani]

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

Vladimir Lomov <v.p.lomov@yandex.ru> schrieb am Sa., 31. Dez. 2016 um
01:44 Uhr:

> ** Philipp Stephani [2016-12-30 17:04:04 +0000]:
>
> > 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.
>
> Thank you, your patch works fine on my example and other real el file.
>
>
Thanks, pushed as 4bbd5424a2290ab4bd88c0af602b7aa7d53a407e.

[-- Attachment #2: Type: text/html, Size: 1975 bytes --]