From 393cd12a5bd73aa2a0ef9f40e1c4db62b1219241 Mon Sep 17 00:00:00 2001 From: Philipp Stephani 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