bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[OGAWA Hirofumi]

With the following test,

(with-temp-buffer
  (select-window (display-buffer (current-buffer)))
  (insert "appl")
  (flyspell-buffer)
  (flyspell-check-previous-highlighted-word))

`flyspell-check-previous-highlighted-word' calls `(error)'.  But "appl"
is the typo that should be fixed.

At the following [**] mark in a that command, the ">" looks not allowing
a typo at (point-min). Maybe the ">" should be the ">=".

(defun flyspell-check-previous-highlighted-word (&optional arg)

[...]

	    (if (> pos (point-min))      <- [**]

[...]

	(save-excursion
	  (goto-char pos)
	  (ispell-word)
	  (setq flyspell-word-cache-word nil) ;; Force flyspell-word re-check
	  (flyspell-word))
      (error "No word to correct before point"))))



In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0)
 of 2020-03-04 built on devron
Repository revision: cf45e8022ee182529668c0d50d27b4e168331e97
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12007000
System Description: Debian GNU/Linux bullseye/sid

Recent messages:
Checking new news...
Reading active file via nnnil...done
nnimap read 0k from server.parknet.ne.jp
Reading active file via nndraft...done
Checking new news...done
Checking new news...
Reading active file via nnnil...done
nnimap read 0k from server.parknet.ne.jp
Reading active file via nndraft...done
Checking new news...done

Configured using:
 'configure --libexecdir=/usr/local/lib --with-x --with-x-toolkit=gtk3
 --without-xim --with-xpm --with-jpeg --with-tiff --with-gif --with-png
 --with-rsvg --with-dbus --with-imagemagick --with-wide-int
 --with-modules'

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

Important settings:
  value of $LANG: ja_JP.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Group

Minor modes in effect:
  gnus-topic-mode: t
  gnus-undo-mode: t
  flycheck-pos-tip-mode: t
  global-flycheck-mode: t
  auto-insert-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  global-company-mode: t
  company-mode: t
  savehist-mode: t
  icomplete-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t

Load-path shadows:
None found.

Features:
(shadow bbdb-message mailalias nnir emacsbug sendmail sort gnus-cite
mm-archive mail-extr gnus-async gnus-bcklg bbdb-gnus-aux qp gnus-ml
disp-table hl-line elfeed-show elfeed-search bookmark elfeed-csv elfeed
elfeed-curl elfeed-log elfeed-db elfeed-lib avl-tree url-queue xml-query
gnus-topic pp url-http url-gw url-cache url-auth utf-7 warnings epa-file
gnutls network-stream nsm nnfolder bbdb-gnus nnnil bbdb-mua spam
spam-stat bbdb-com crm bbdb bbdb-site timezone gnus-uu yenc gnus-demon
gnus-delay gnus-draft gnus-agent gnus-srvr gnus-score score-mode
nnvirtual nntp gnus-cache gnus-msg gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum shr svg xml dom nndraft nnmh gnus-group
gnus-undo gnus-xoauth2 oauth2-ext plstore gnus-start gnus-cloud nnimap
nnmail mail-source utf7 netrc nnoo parse-time iso8601 gnus-spec gnus-int
gnus-range message rmc puny format-spec rfc822 mml mml-sec epa derived
epg epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader gnus-win gnus nnheader gnus-util rmail
rmail-loaddefs rfc2047 rfc2045 ietf-drums text-property-search time-date
mail-utils mm-util mail-prsvr wid-edit dircolors-faces dired-x dired
dired-loaddefs company-yasnippet flyspell ispell server generic-x
langtool-autoloads multi-translate google-translate-smooth-ui
google-translate google-translate-default-ui google-translate-core-ui
ido google-translate-core google-translate-tk google-translate-backend
url url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap git-modes-autoloads flycheck-relint relint
xr flycheck-pos-tip pos-tip flycheck find-func rx dash autoinsert
cl-extra yasnippet help-mode company-oddmuse company-keywords
company-etags etags fileloop generator xref project company-gtags
company-dabbrev-code company-dabbrev company-files company-capf
company-cmake company-xcode company-clang company-semantic company-eclim
company-template company-bbdb company edmacro kmacro pcase bbdb-loaddefs
auth-source-pass irfc-autoloads grep compile comint ansi-color ring
savehist browse-kill-ring delsel tab-bar-session desktop frameset
icomplete paren mozc-popup popup mozc-im-indicater mozc-im advice info
mozc package easymenu browse-url url-handlers url-parse auth-source
cl-seq eieio eieio-core cl-macs eieio-loaddefs password-cache json
subr-x map url-vars seq byte-opt gv bytecomp byte-compile cconv
cl-loaddefs cl-lib japan-util 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 timer select scroll-bar mouse
jit-lock font-lock syntax facemenu 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 loaddefs button faces
cus-face macroexp files 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 emacs)

Memory information:
((conses 16 808695 2116413)
 (symbols 48 25847 11)
 (strings 32 1573255 20052)
 (string-bytes 1 63713301)
 (vectors 16 478989)
 (vector-slots 8 5015592 67198)
 (floats 8 142327 212)
 (intervals 56 1036 276)
 (buffers 1000 28))

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Stefan Kangas]

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

tags 39898 + patch
thanks

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> With the following test,
>
> (with-temp-buffer
>   (select-window (display-buffer (current-buffer)))
>   (insert "appl")
>   (flyspell-buffer)
>   (flyspell-check-previous-highlighted-word))
>
> `flyspell-check-previous-highlighted-word' calls `(error)'.  But "appl"
> is the typo that should be fixed.
>
> At the following [**] mark in a that command, the ">" looks not allowing
> a typo at (point-min). Maybe the ">" should be the ">=".
>
> (defun flyspell-check-previous-highlighted-word (&optional arg)
>
> [...]
>
> 	    (if (> pos (point-min))      <- [**]
>
> [...]
>
> 	(save-excursion
> 	  (goto-char pos)
> 	  (ispell-word)
> 	  (setq flyspell-word-cache-word nil) ;; Force flyspell-word re-check
> 	  (flyspell-word))
>       (error "No word to correct before point"))))

Indeed, this just looks like an OBOE to me.  The attached patch fixes it
as per your suggestion.

However, there is some stuff going on with `previous-overlay-change', so
it would be good if someone could double-check that the fix here is
correct.

AFAIU, this should work at the price that in a buffer with no misspelled
words we uselessly check if there's a misspelled word at point-min.

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-Fix-OBOE-in-flyspell-check-previous-highlighted-word.patch --]
[-- Type: text/x-diff, Size: 991 bytes --]

From a9723e982188034b2b0a52a8500d58497f96cf18 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Thu, 27 Aug 2020 21:21:03 +0200
Subject: [PATCH] Fix OBOE in flyspell-check-previous-highlighted-word

* lisp/textmodes/flyspell.el
(flyspell-check-previous-highlighted-word): Fix off-by-one error when
word is at (point-min).  (Bug#39898)

Suggested by OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>.
---
 lisp/textmodes/flyspell.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 9805928721..6889d7eada 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -1904,7 +1904,7 @@ flyspell-check-previous-highlighted-word
 	  (while (and (setq pos (previous-overlay-change pos))
 		      (not (= pos pos1)))
 	    (setq pos1 pos)
-	    (if (> pos (point-min))
+	    (if (>= pos (point-min))
 		(progn
 		  (setq ovs (overlays-at pos))
 		  (while (consp ovs)
-- 
2.28.0

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 27 Aug 2020 12:37:45 -0700
> Cc: 39898@debbugs.gnu.org
> 
> > With the following test,
> >
> > (with-temp-buffer
> >   (select-window (display-buffer (current-buffer)))
> >   (insert "appl")
> >   (flyspell-buffer)
> >   (flyspell-check-previous-highlighted-word))
> >
> > `flyspell-check-previous-highlighted-word' calls `(error)'.  But "appl"
> > is the typo that should be fixed.
> >
> > At the following [**] mark in a that command, the ">" looks not allowing
> > a typo at (point-min). Maybe the ">" should be the ">=".
> >
> > (defun flyspell-check-previous-highlighted-word (&optional arg)
> >
> > [...]
> >
> > 	    (if (> pos (point-min))      <- [**]
> >
> > [...]
> >
> > 	(save-excursion
> > 	  (goto-char pos)
> > 	  (ispell-word)
> > 	  (setq flyspell-word-cache-word nil) ;; Force flyspell-word re-check
> > 	  (flyspell-word))
> >       (error "No word to correct before point"))))
> 
> Indeed, this just looks like an OBOE to me.

Does it?  The doc string of flyspell-check-previous-highlighted-word
says:

  Correct the closest previous word that is highlighted as misspelled.
  This function scans for a word which starts before point that has been
			    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  highlighted by Flyspell as misspelled.

The test case, AFAIU, creates a situation where the misspelled word
starts _after_ point (remember that point is a place between 2
characters, and its value is the character following point).  So I
don't see a bug here.  Am I missing something?

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[OGAWA Hirofumi]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@marxist.se>
>> Date: Thu, 27 Aug 2020 12:37:45 -0700
>> Cc: 39898@debbugs.gnu.org
>> 
>> Indeed, this just looks like an OBOE to me.
>
> Does it?  The doc string of flyspell-check-previous-highlighted-word
> says:
>
>   Correct the closest previous word that is highlighted as misspelled.
>   This function scans for a word which starts before point that has been
> 			    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   highlighted by Flyspell as misspelled.
>
> The test case, AFAIU, creates a situation where the misspelled word
> starts _after_ point (remember that point is a place between 2
> characters, and its value is the character following point).  So I
> don't see a bug here.  Am I missing something?

(with-temp-buffer
  (select-window (display-buffer (current-buffer)))
  (insert "appl")
  (insert "   ")
  (flyspell-buffer)
  (flyspell-check-previous-highlighted-word))

Then, this point is before, and should work? This also calls
(error "No word...").

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> The test case, AFAIU, creates a situation where the misspelled word
> starts _after_ point (remember that point is a place between 2
> characters, and its value is the character following point).  So I
> don't see a bug here.  Am I missing something?

No, the misspelled word starts before point.  Note that `insert' moves
point to after the inserted text.

(with-temp-buffer
  (select-window (display-buffer (current-buffer)))
  (insert "appl")
  (message "%s" (point)) ; 5
  (flyspell-buffer)
  (flyspell-check-previous-highlighted-word))

The problem is there even if you type a misspelled word manually at
(point-min) and run `flyspell-check-previous-highlighted-word'.

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Eli Zaretskii]

> From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> Cc: Stefan Kangas <stefan@marxist.se>, 39898@debbugs.gnu.org
> Date: Fri, 28 Aug 2020 16:02:59 +0900
> 
> > The test case, AFAIU, creates a situation where the misspelled word
> > starts _after_ point (remember that point is a place between 2
> > characters, and its value is the character following point).  So I
> > don't see a bug here.  Am I missing something?
> 
> (with-temp-buffer
>   (select-window (display-buffer (current-buffer)))
>   (insert "appl")
>   (insert "   ")
>   (flyspell-buffer)
>   (flyspell-check-previous-highlighted-word))
> 
> Then, this point is before, and should work? This also calls
> (error "No word...").

Fair enough, but with the change you proposed, what happens if the
command is invoked with point at BOB?

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Fair enough, but with the change you proposed, what happens if the
> command is invoked with point at BOB?

You get "No word to correct before point".

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 28 Aug 2020 04:27:20 -0700
> Cc: 39898@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Fair enough, but with the change you proposed, what happens if the
> > command is invoked with point at BOB?
> 
> You get "No word to correct before point".

Then I guess my objections are null and void.

bug#39898: 28.0.50; The off-by-one bug in `flyspell-check-previous-highlighted-word'

[Stefan Kangas]

close 39898 28.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

>> > Fair enough, but with the change you proposed, what happens if the
>> > command is invoked with point at BOB?
>>
>> You get "No word to correct before point".
>
> Then I guess my objections are null and void.

Thanks, now pushed to master as commit dd2c37d0e1.  Closing.

Best regards,
Stefan Kangas