bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Manuel Uberti]

This is a small recipe to reproduce the problem:

- emacs -Q
- Write: (y-or-n-p "Is this a test?")
- M-x flymake-mode
- M-x flymake-show-buffer-diagnostics
- You will see the following message: `y-or-n-p' argument should end
with "? "

According to this item in NEWS I was led to believe that the trailing
whitespace was not needed any more:

** 'yes-or-no-p' and 'y-or-n-p' PROMPT parameter no longer needs trailing space.
This has been the case since Emacs 24.4 but was not announced or
documented until now.


In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.18, cairo 
version 1.16.0)
  of 2021-09-16 built on hathaway
Repository revision: db12b386537f0a0d36a4759c625ba643e391e4f2
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12009000
System Description: Ubuntu 20.04 LTS

Configured using:
  'configure --with-harfbuzz --with-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM GTK3 ZLIB

Important settings:
   value of $LC_MESSAGES: en_GB.UTF-8
   value of $LC_MONETARY: it_IT.UTF-8
   value of $LC_NUMERIC: it_IT.UTF-8
   value of $LC_TIME: it_IT.UTF-8
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
   flymake-mode: t
   tooltip-mode: t
   global-eldoc-mode: t
   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
   indent-tabs-mode: t
   transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source eieio eieio-core eieio-loaddefs
password-cache json map mm-decode mm-bodies mm-encode mailabbrev
gmm-utils mailheader sendmail mail-utils time-date noutline outline
checkdoc comp comp-cstr subr-x rx cl-extra lisp-mnt mail-parse rfc2231
rfc2047 rfc2045 mm-util ietf-drums mail-prsvr help-mode cl-macs cl-seq
flymake-proc flymake project compile text-property-search comint
ansi-color ring warnings thingatpt misearch multi-isearch vc-git
diff-mode easy-mmode vc-dispatcher cl-loaddefs cl-lib seq byte-opt gv
bytecomp byte-compile cconv iso-transl tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer 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 composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 112572 7925)
  (symbols 48 9674 1)
  (strings 32 28529 1758)
  (string-bytes 1 1055644)
  (vectors 16 19978)
  (vector-slots 8 337135 14232)
  (floats 8 37 28)
  (intervals 56 342 11)
  (buffers 992 16))

-- 
Manuel Uberti
www.manueluberti.eu

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Manuel Uberti]

M-x checkdoc complains about this as well, so I guess it is more a checkdoc than 
a Flymake problem here.

-- 
Manuel Uberti
www.manueluberti.eu

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Lars Ingebrigtsen]

Manuel Uberti <manuel.uberti@inventati.org> writes:

> M-x checkdoc complains about this as well, so I guess it is more a
> checkdoc than a Flymake problem here.

Yeah, I guess all of the y-or-n-p stuff should be removed from
checkdoc.el?  Or should it still verify that the string ends with a
question mark?  I've added Stefan to the CCs; perhaps he has an opinion
here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> M-x checkdoc complains about this as well, so I guess it is more a
>> checkdoc than a Flymake problem here.
>
> Yeah, I guess all of the y-or-n-p stuff should be removed from
> checkdoc.el?  Or should it still verify that the string ends with a
> question mark?  I've added Stefan to the CCs; perhaps he has an opinion
> here.

As the NEWS item points out, the behavior was changed [IMO for the
better] already in 2014 (commit 298520dfb7f6). The question now is what
to do with Flymake/checkdoc, and I see three ways forward:

 A) We remove this check.
 B) We change it to only warn about a missing question mark.
 C) We change it as in B, but also warn about a spurious space.

Alternative A is the easiest one, but loses functionality.

Alternative B is better, but leaves a spurious space where it's not
needed.

Alternative C will cause some code-churn, but is also formally the most
correct one. It might also save someones sanity if they are trying to
fit a line into a 80 or 100 character limit.

My preference is for, in order: C > B > A.

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Alternative C will cause some code-churn, but is also formally the most
> correct one. It might also save someones sanity if they are trying to
> fit a line into a 80 or 100 character limit.
>
> My preference is for, in order: C > B > A.

I think C is too ambitious -- there's code out-of-tree that needs to
work in a wide range of Emacs versions, so they can't remove the
trailing space.  So I like B (i.e.., check that there's "\\? *" at the
end).

(And then we can remove trailing spaces in-tree at will.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Stefan Kangas]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Alternative C will cause some code-churn, but is also formally the most
>> correct one. It might also save someones sanity if they are trying to
>> fit a line into a 80 or 100 character limit.
>>
>> My preference is for, in order: C > B > A.
>
> I think C is too ambitious -- there's code out-of-tree that needs to
> work in a wide range of Emacs versions, so they can't remove the
> trailing space.  So I like B (i.e.., check that there's "\\? *" at the
> end).

Sounds good.  How about the attached patch?

[-- Attachment #2: 0001-checkdoc-y-or-n-p-no-longer-needs-space.patch --]
[-- Type: text/x-diff, Size: 6710 bytes --]

From 60d56c427511a5946f823c4249d9af3421288844 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Thu, 16 Sep 2021 19:13:56 +0200
Subject: [PATCH] checkdoc: 'y-or-n-p' no longer needs space

* lisp/emacs-lisp/checkdoc.el (checkdoc-message-text-engine):
Change 'y-or-n-p' check to accept prompt ending with both "? " or "?",
that is, it no longer needs the space.  (Bug#50621)
(checkdoc--fix-y-or-n-p): New helper function.
* test/lisp/emacs-lisp/checkdoc-tests.el (checkdoc-tests-fix-y-or-n-p)
(checkdoc-tests-fix-y-or-n-p/no-change)
(checkdoc-tests-fix-y-or-n-p/with-space): New tests.
---
 lisp/emacs-lisp/checkdoc.el            | 82 ++++++++++----------------
 test/lisp/emacs-lisp/checkdoc-tests.el | 30 ++++++++++
 2 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 01f2c0d95f..f8df223ce9 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -2475,6 +2475,31 @@ checkdoc-message-text-next-string
 	  (setq return type))))
     return))
 
+(defun checkdoc--fix-y-or-n-p ()
+  "Fix `y-or-n-p' prompt to end with \"?\" or \"? \".
+The space is technically redundant, but also more compatible with
+Emacs versions before Emacs 24.1.  In the future, we might treat
+a space as a style error."
+  (when (and (save-excursion (forward-sexp 1)
+                             (forward-char -3)
+                             (not (looking-at "\\? ")))
+             (save-excursion (forward-sexp 1)
+                             (forward-char -2)
+                             (not (looking-at "\\?"))))
+    (if (and
+         (save-excursion (forward-sexp 1)
+                         (forward-char -1)
+                         (looking-at "\""))
+         (checkdoc-autofix-ask-replace
+          (match-beginning 0) (match-end 0)
+          (format-message
+           "`y-or-n-p' argument should end with \"? \".  Fix?")
+          "?\"" t))
+        nil
+      (checkdoc-create-error
+       "`y-or-n-p' argument should end with \"?\""
+       (match-beginning 0) (match-end 0)))))
+
 (defun checkdoc-message-text-engine (&optional type)
   "Return or fix errors found in strings passed to a message display function.
 According to the documentation for the function `error', the error list
@@ -2530,63 +2555,20 @@ checkdoc-message-text-engine
 	  "Error messages should *not* end with a period"
 	  (match-beginning 0) (match-end 0))
        nil)
-     ;; `y-or-n-p' documentation explicitly says:
-     ;; It should end in a space; `y-or-n-p' adds `(y or n) ' to it.
-     ;; I added the ? requirement.  Without it, it is unclear that we
-     ;; ask a question and it appears to be an undocumented style.
-     (if (eq type 'y-or-n-p)
-	 (if (not (save-excursion (forward-sexp 1)
-				  (forward-char -3)
-				  (not (looking-at "\\? "))))
-	     nil
-	   (if (save-excursion (forward-sexp 1)
-			       (forward-char -2)
-			       (looking-at "\\?"))
-	       ;; If we see a ?, then replace with "? ".
-	       (if (checkdoc-autofix-ask-replace
-		    (match-beginning 0) (match-end 0)
-                    (format-message
-                     "`y-or-n-p' argument should end with \"? \".  Fix? ")
-		    "? " t)
-		   nil
-		 (checkdoc-create-error
-		  "`y-or-n-p' argument should end with \"? \""
-		  (match-beginning 0) (match-end 0)))
-	     (if (save-excursion (forward-sexp 1)
-				 (forward-char -2)
-				 (looking-at " "))
-		 (if (checkdoc-autofix-ask-replace
-		      (match-beginning 0) (match-end 0)
-                      (format-message
-                       "`y-or-n-p' argument should end with \"? \".  Fix? ")
-		      "? " t)
-		     nil
-		   (checkdoc-create-error
-		    "`y-or-n-p' argument should end with \"? \""
-		    (match-beginning 0) (match-end 0)))
-	       (if (and ;; if this isn't true, we have a problem.
-		    (save-excursion (forward-sexp 1)
-				    (forward-char -1)
-				    (looking-at "\""))
-		    (checkdoc-autofix-ask-replace
-		     (match-beginning 0) (match-end 0)
-                     (format-message
-                      "`y-or-n-p' argument should end with \"? \".  Fix? ")
-		     "? \"" t))
-		   nil
-		 (checkdoc-create-error
-		  "`y-or-n-p' argument should end with \"? \""
-		  (match-beginning 0) (match-end 0)))))))
+     ;; From `(elisp) Programming Tips': "A question asked in the
+     ;; minibuffer with `yes-or-no-p' or `y-or-n-p' should start with
+     ;; a capital letter and end with '?'."
+     (when (eq type 'y-or-n-p)
+       (checkdoc--fix-y-or-n-p))
      ;; Now, let's just run the spell checker on this guy.
      (checkdoc-ispell-docstring-engine (save-excursion (forward-sexp 1)
-						       (point)))
-     )))
+                                                       (point))))))
 
 ;;; Auto-fix helper functions
 ;;
 (defun checkdoc-y-or-n-p (question)
   "Like `y-or-n-p', but pays attention to `checkdoc-autofix-flag'.
-Argument QUESTION is the prompt passed to `y-or-n-p'."
+  Argument QUESTION is the prompt passed to `y-or-n-p'."
   (prog1
       (if (or (not checkdoc-autofix-flag)
 	      (eq checkdoc-autofix-flag 'never))
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index a4b252031f..3eb7da3d4a 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -146,4 +146,34 @@ checkdoc-tests-in-abbrevation-p/with-escaped-parens
     (re-search-forward "e.g")
     (should (checkdoc-in-abbreviation-p (point)))))
 
+(ert-deftest checkdoc-tests-fix-y-or-n-p ()
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((standard-output (current-buffer))
+          (checkdoc-autofix-flag 'automatic))
+      (prin1 '(y-or-n-p "foo"))         ; "foo"
+      (goto-char (length "(y-or-n-p "))
+      (checkdoc--fix-y-or-n-p)
+      (should (equal (buffer-string) "(y-or-n-p \"foo?\")")))))
+
+(ert-deftest checkdoc-tests-fix-y-or-n-p/no-change ()
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((standard-output (current-buffer))
+          (checkdoc-autofix-flag 'automatic))
+      (prin1 '(y-or-n-p "foo?"))        ; "foo?"
+      (goto-char (length "(y-or-n-p "))
+      (checkdoc--fix-y-or-n-p)
+      (should (equal (buffer-string) "(y-or-n-p \"foo?\")")))))
+
+(ert-deftest checkdoc-tests-fix-y-or-n-p/with-space ()
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((standard-output (current-buffer))
+          (checkdoc-autofix-flag 'automatic))
+      (prin1 '(y-or-n-p "foo? "))       ; "foo? "
+      (goto-char (length "(y-or-n-p "))
+      (checkdoc--fix-y-or-n-p)
+      (should (equal (buffer-string) "(y-or-n-p \"foo? \")")))))
+
 ;;; checkdoc-tests.el ends here
-- 
2.30.2

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Sounds good.  How about the attached patch?

Looks good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Sounds good.  How about the attached patch?
>
> Looks good to me.

Thanks, pushed to master as commit b6bff3ba79.

Closing this bug.  If anyone is still seeing problems, please report
back and we can re-open.

bug#50621: 28.0.50; Flymake wrong message about y-or-n-p

[Manuel Uberti]

On 17/09/21 22:41, Stefan Kangas wrote:
> Thanks, pushed to master as commit b6bff3ba79.
> 
> Closing this bug.  If anyone is still seeing problems, please report
> back and we can re-open.

Thank you, Stefan, checkdoc does not complain any more.

-- 
Manuel Uberti
www.manueluberti.eu