bug#22097: Ispell: lazy highlighting doesn't work properly.

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Alan Mackenzie]

Hello, Emacs.

With Emacs 24.5, or a recent emacs-25 state, do:

Set up ispell configuration such that "xdisp" is not a known word.

emacs -Q
C-x C-f src/xdisp.c
Adjust frame/window configuration such that the buffer is displayed in a
  window at least 50 lines high.
M-x ispell

Press the spacebar several times until "xdisp" on line 34 is highlighted.
Notice that the next occurrence of "xdisp" (L48, in the diagram) hasn't
been lazily highlighted.  This is a bug.

Press the spacebar several times more until "xdisp" on L48 is
highlighted.  The occurrence on L34 is now lazily highlighted as it
should be.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Eli Zaretskii]

> Date: Sat, 5 Dec 2015 11:42:30 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> emacs -Q
> C-x C-f src/xdisp.c
> Adjust frame/window configuration such that the buffer is displayed in a
>   window at least 50 lines high.
> M-x ispell
> 
> Press the spacebar several times until "xdisp" on line 34 is highlighted.
> Notice that the next occurrence of "xdisp" (L48, in the diagram) hasn't
> been lazily highlighted.  This is a bug.
> 
> Press the spacebar several times more until "xdisp" on L48 is
> highlighted.  The occurrence on L34 is now lazily highlighted as it
> should be.

This is a "feature".  Ispell skips regions determined by certain
regexps, see ispell-skip-region-alist.  In this case, the ASCII-art
diagram around the second occurrence of "xdisp" includes regions that
match some of the regexps (AFAICT, the '+' character followed and
preceded by '-' triggers that).  Lazy highlight doesn't want to try
matching in those skip-regions, and since it cannot support
non-contiguous regions, it simply limits itself to the beginning of
the first such region, which happens to be before the second "xdisp".

IOW, the lazy highlight, as coded, should not be expected to highlight
all the matching misspellings in such situations.

If we think this is a grave limitation (do we?), then we need to
invoke isearch-lazy-highlight-new-loop more than once, until we get to
the end of the window.  I don't know enough about
isearch-lazy-highlight-new-loop to tell if this will work, but
hopefully someone else here will.

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Alan Mackenzie]

Hello, Eli.

On Sat, Dec 05, 2015 at 03:23:42PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 5 Dec 2015 11:42:30 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > emacs -Q
> > C-x C-f src/xdisp.c
> > Adjust frame/window configuration such that the buffer is displayed in a
> >   window at least 50 lines high.
> > M-x ispell

> > Press the spacebar several times until "xdisp" on line 34 is highlighted.
> > Notice that the next occurrence of "xdisp" (L48, in the diagram) hasn't
> > been lazily highlighted.  This is a bug.

> > Press the spacebar several times more until "xdisp" on L48 is
> > highlighted.  The occurrence on L34 is now lazily highlighted as it
> > should be.

> This is a "feature".  Ispell skips regions determined by certain
> regexps, see ispell-skip-region-alist.  In this case, the ASCII-art
> diagram around the second occurrence of "xdisp" includes regions that
> match some of the regexps (AFAICT, the '+' character followed and
> preceded by '-' triggers that).  Lazy highlight doesn't want to try
> matching in those skip-regions, and since it cannot support
> non-contiguous regions, it simply limits itself to the beginning of
> the first such region, which happens to be before the second "xdisp".

Ah.  I didn't know this.

> IOW, the lazy highlight, as coded, should not be expected to highlight
> all the matching misspellings in such situations.

However, the bug manifests itself a bit later on in plain text.

There's a paragraph starting at L199 about bidi.  After several more
hits on the space bar, the first occurrence of "bidi" (L201) gets
highlighted; the second occurrence (on the same line) gets lazily
highlighted.  The third (L204) and fourth (L205) remain unhighlighted.

Hit the spacebar another time.  All four occurrences are now
highlighted.

As far as I can see, there's nothing remotely ASCII-arty in that
paragraph.  Unless the "---" sequences are somehow being interpreted as
ASCII-art.

I'll look into this.

> If we think this is a grave limitation (do we?), then we need to
> invoke isearch-lazy-highlight-new-loop more than once, until we get to
> the end of the window.  I don't know enough about
> isearch-lazy-highlight-new-loop to tell if this will work, but
> hopefully someone else here will.

This would require quite a bit of modification to
isearch-lazy-highlight-new-loop, and possibly its callers, which I would
guess to be more work than it's worth, at the moment.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Eli Zaretskii]

> Date: Sat, 5 Dec 2015 14:06:09 +0000
> Cc: 22097@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > This is a "feature".  Ispell skips regions determined by certain
> > regexps, see ispell-skip-region-alist.  In this case, the ASCII-art
> > diagram around the second occurrence of "xdisp" includes regions that
> > match some of the regexps (AFAICT, the '+' character followed and
> > preceded by '-' triggers that).  Lazy highlight doesn't want to try
> > matching in those skip-regions, and since it cannot support
> > non-contiguous regions, it simply limits itself to the beginning of
> > the first such region, which happens to be before the second "xdisp".
> 
> Ah.  I didn't know this.

Neither did I.  I just traced through the code, is all.

> However, the bug manifests itself a bit later on in plain text.
> 
> There's a paragraph starting at L199 about bidi.  After several more
> hits on the space bar, the first occurrence of "bidi" (L201) gets
> highlighted; the second occurrence (on the same line) gets lazily
> highlighted.  The third (L204) and fourth (L205) remain unhighlighted.
> 
> Hit the spacebar another time.  All four occurrences are now
> highlighted.
> 
> As far as I can see, there's nothing remotely ASCII-arty in that
> paragraph.  Unless the "---" sequences are somehow being interpreted as
> ASCII-art.

ispell-skip-region-alist is a complex regexp, something there must've
(mis)fired.

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Alan Mackenzie]

Hello, Eli.

On Sat, Dec 05, 2015 at 04:20:38PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 5 Dec 2015 14:06:09 +0000
> > Cc: 22097@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

[ .... ]

> > However, the bug manifests itself a bit later on in plain text.

> > There's a paragraph starting at L199 about bidi.  After several more
> > hits on the space bar, the first occurrence of "bidi" (L201) gets
> > highlighted; the second occurrence (on the same line) gets lazily
> > highlighted.  The third (L204) and fourth (L205) remain unhighlighted.

> > Hit the spacebar another time.  All four occurrences are now
> > highlighted.

> > As far as I can see, there's nothing remotely ASCII-arty in that
> > paragraph.  Unless the "---" sequences are somehow being interpreted as
> > ASCII-art.

> ispell-skip-region-alist is a complex regexp, something there must've
> (mis)fired.

I've got a little tool that dumps regexps in a more readable form.  Here
is what it makes of ispell-skip-region-alist:



\(   \|  \|                                                                         \)
  --+  _+  \(   \|                      \)\(  \|    \)*\(                        \)+
             /\w  \(                  \)    \w  [-_]     [.:/@]+\(  \|        \)+
                    \(  \|    \)+[.:@]                            \w  [-_~=?&]
                      \w  [-_]


Clearly, the "---"s are going to trigger the very first alternative of
the regexp.  I don't think "bidi.c", of itself, triggers the regexp.

The first two alternatives were added in "for performance reasons" for
when "-" or "_" are part of word syntax.  In otherwords, "\w\|[-_]" was
leading to exponential degradation in these circumstances.

However, nowadays we've got "\s_", which probably didn't exist when
ispell.el was written.  We could reformulate the regexp using it, which
might allow us to get rid of the "--+" and "_+" alternatives.

-- 
Alan Mackenzie (Nuremberg, Germany).
 

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Eli Zaretskii]

> Date: Sat, 5 Dec 2015 16:04:29 +0000
> Cc: 22097@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Clearly, the "---"s are going to trigger the very first alternative of
> the regexp.  I don't think "bidi.c", of itself, triggers the regexp.

Not bidi.c itself, some text before it.  E.g., I see several counts of
"---" there, which just might do the trick.

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Juri Linkov]

> \(   \|  \|                                                                         \)
>   --+  _+  \(   \|                      \)\(  \|    \)*\(                        \)+
>              /\w  \(                  \)    \w  [-_]     [.:/@]+\(  \|        \)+
>                     \(  \|    \)+[.:@]                            \w  [-_~=?&]
>                       \w  [-_]

This is a nice ASCII-art in itself :-), but the problem is in another place -
in ispell-highlight-spelling-error-overlay you can see word boundaries
"\\b" that were added for every search string to not highlight parts
of words, such as when the current check is for e.g. "init", we don't
want to highlight "init" in the words "initialize", "initially", etc.
The drawback of this approach is that it depends on the word syntax in a
particular mode.

Another limitation is limiting lazy-highlighting by reg-start/reg-end
instead of a wider area of window-start/window-end.

Finally, we need now here this fix:

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index fe27f0f..aa51446 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -2809,6 +2809,7 @@ ispell-highlight-spelling-error-overlay
 		  (regexp-quote (buffer-substring-no-properties start end))
 		  "\\b"))
 		(isearch-regexp t)
+		(isearch-regexp-function nil)
 		(isearch-case-fold-search nil)
 		(isearch-forward t)
 		(isearch-other-end start)

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Juri Linkov]

> Another limitation is limiting lazy-highlighting by reg-start/reg-end
> instead of a wider area of window-start/window-end.

This patch addresses this limitation: it limits the highlighting area
either to the region boundaries, or to the window boundaries.

diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index aa51446..82c589d 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -2786,6 +2786,8 @@ ispell-highlight-spelling-error-xemacs
   ;;(sit-for 0)
   )
 
+(defvar ispell-reg-start nil)
+(defvar ispell-reg-end nil)
 
 (defun ispell-highlight-spelling-error-overlay (start end &optional highlight)
   "Highlight the word from START to END using overlays.
@@ -2815,8 +2817,8 @@ ispell-highlight-spelling-error-overlay
 		(isearch-other-end start)
 		(isearch-error nil))
 	    (isearch-lazy-highlight-new-loop
-	     (if (boundp 'reg-start) reg-start)
-	     (if (boundp 'reg-end)   reg-end)))
+	     (if (not (eq ispell-reg-start (point-min))) ispell-reg-start (window-start))
+	     (if (not (eq ispell-reg-end   (point-max))) ispell-reg-end   (window-end))))
 	(lazy-highlight-cleanup lazy-highlight-cleanup)
 	(setq isearch-lazy-highlight-last-string nil))))
 
@@ -3211,6 +3213,7 @@ ispell-region
 amount for last line processed."
   (interactive "r")			; Don't flag errors on read-only bufs.
   (ispell-set-spellchecker-params)      ; Initialize variables and dicts alists
+  (setq ispell-reg-start reg-start ispell-reg-end reg-end)
   (if (not recheckp)
       (ispell-accept-buffer-local-defs)) ; set up dictionary, local words, etc.
   (let ((skip-region-start (make-marker))

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Alan Mackenzie]

Hello, Juri.

On Sun, Dec 06, 2015 at 01:04:15AM +0200, Juri Linkov wrote:
[quoting removed for clarity]
\(   \|  \|                                                                         \)
  --+  _+  \(   \|                      \)\(  \|    \)*\(                        \)+
             /\w  \(                  \)    \w  [-_]     [.:/@]+\(  \|        \)+
                    \(  \|    \)+[.:@]                            \w  [-_~=?&]
                      \w  [-_]

rrrrrrrrrrr         111111111111           22222222222          3333333333333333    rr

> This is a nice ASCII-art in itself :-), but the problem is in another place -

There are problems in that regexp, too:
(i) As noted in the comment in the source, when - or _ has word syntax,
the regexp is ill-formed - there is ambiguity in \(\w\|[-_]\) because the
- or _ can match either side of the regexp fragment, leading to bad
performance.  This happens in the fragments maked 1, 2, and 3.
(ii) The matching on --+ or _+ damage the lazy highlighting, often
preventing it happening.  But these clauses are there solely to protect
against the poor performance noted in (i).

I have solved these problems by generating this regexp at runtime,
properly taking into account the syntax table.  The fragments 1, 2, and 3
are generated at runtime, and fragments r are now redundant and have been
removed.  This regexp is no longer an element of
ispell-skip-region-alist, instead being dynamically generated wherever
ispell-skip-region-alist is used (3 places).

I haven't yet tried the two patches you sent me.  The one binding
isearch-regexp-function to nil looks obvious and straightforward, the
other one I'll need to look at more carefully.

Here is the current version of my patch for this change.  The comments
and doc strings in it probably are of too low quality to be committed.

[ .... ]



diff --git a/lisp/textmodes/ispell.el b/lisp/textmodes/ispell.el
index 7d5bb6d..9695a6b 100644
--- a/lisp/textmodes/ispell.el
+++ b/lisp/textmodes/ispell.el
@@ -1782,6 +1782,49 @@ ispell-parsing-keyword
 a `~' followed by an extended-character mode -- such as `~.tex'.
 The last occurring definition in the buffer will be used.")
 
+(defun ispell--\\w-filter (char)
+  "Return CHAR in a string when CHAR doesn't have \"word\" syntax,
+nil otherwise.  CHAR must be a character."
+  (let ((str (string char)))
+    (and
+     (not (string-match "\\w" str))
+     str)))
+
+(defun ispell--make-\\w-expression (chars)
+  "Make an expression like \"\\(\\w\\|[-_]\\)\".
+This (parenthesized) expression matches either a character of
+\"word\" syntax or one in CHARS.
+
+CHARS is a string of characters.  A member of CHARS is omitted
+from the expression if it already has word syntax.  (Be sensible
+about special characters such as ?\\, ?^, ?], and ?- in CHARS.)
+If after this filtering there are no chars left, or only one, a
+special form of the expression is generated."
+  (let ((filtered
+	 (mapconcat #'ispell--\\w-filter chars "")))
+    (concat
+     "\\(\\w"
+     (cond
+      ((equal filtered "")
+       "\\)")
+      ((eq (length filtered) 1)
+       (concat "\\|" filtered "\\)"))
+      (t
+       (concat "\\|[" filtered "]\\)"))))))
+
+(defun ispell--make-filename-or-URL-re ()
+  "Construct a regexp to match some file names or URLs or email addresses."
+  (concat ;"\\(--+\\|_+\\|"
+          "\\(/\\w\\|\\("
+          (ispell--make-\\w-expression "-_")
+          "+[.:@]\\)\\)"
+          (ispell--make-\\w-expression "-_")
+          "*\\([.:/@]+"
+          (ispell--make-\\w-expression "-_~=?&")
+          "+\\)+"
+          ;"\\)"
+          ))
+
 ;;;###autoload
 (defvar ispell-skip-region-alist
   `((ispell-words-keyword	   forward-line)
@@ -1798,7 +1841,7 @@ ispell-skip-region-alist
     ;; Matches e-mail addresses, file names, http addresses, etc.  The
     ;; `-+' `_+' patterns are necessary for performance reasons when
     ;; `-' or `_' part of word syntax.
-    (,(purecopy "\\(--+\\|_+\\|\\(/\\w\\|\\(\\(\\w\\|[-_]\\)+[.:@]\\)\\)\\(\\w\\|[-_]\\)*\\([.:/@]+\\(\\w\\|[-_~=?&]\\)+\\)+\\)"))
+;    (,(purecopy "\\(--+\\|_+\\|\\(/\\w\\|\\(\\(\\w\\|[-_]\\)+[.:@]\\)\\)\\(\\w\\|[-_]\\)*\\([.:/@]+\\(\\w\\|[-_~=?&]\\)+\\)+\\)"))
     ;; above checks /.\w sequences
     ;;("\\(--+\\|\\(/\\|\\(\\(\\w\\|[-_]\\)+[.:@]\\)\\)\\(\\w\\|[-_]\\)*\\([.:/@]+\\(\\w\\|[-_~=?&]\\)+\\)+\\)")
     ;; This is a pretty complex regexp.  It can be simplified to the following:
@@ -3387,7 +3430,8 @@ ispell-begin-skip-region-regexp
               (if (string= "" comment-end) "^" (regexp-quote comment-end)))
           (if (and (null ispell-check-comments) comment-start)
               (regexp-quote comment-start))
-          (ispell-begin-skip-region ispell-skip-region-alist)))
+          (ispell-begin-skip-region ispell-skip-region-alist)
+          (ispell--make-filename-or-URL-re)))
    "\\|"))
 
 
@@ -3426,6 +3470,8 @@ ispell-skip-region-list
 The list is of the form described by variable `ispell-skip-region-alist'.
 Must be called after `ispell-buffer-local-parsing' due to dependence on mode."
   (let ((skip-alist ispell-skip-region-alist))
+    (setq skip-alist (append (list (list (ispell--make-filename-or-URL-re)))
+                             skip-alist))
     ;; only additional explicit region definition is tex.
     (if (eq ispell-parser 'tex)
 	(setq case-fold-search nil
@@ -4119,9 +4165,10 @@ ispell-message
 		      (ispell-non-empty-string vm-included-text-prefix)))
 	     (t default-prefix)))
 	   (ispell-skip-region-alist
-	    (cons (list (concat "^\\(" cite-regexp "\\)")
-			(function forward-line))
-		  ispell-skip-region-alist))
+	    (cons (list (ispell--make-filename-or-URL-re))
+                  (cons (list (concat "^\\(" cite-regexp "\\)")
+                              (function forward-line))
+                        ispell-skip-region-alist)))
 	   (old-case-fold-search case-fold-search)
 	   (dictionary-alist ispell-message-dictionary-alist)
 	   (ispell-checking-message t))



-- 
Alan Mackenzie (Nuremberg, Germany).

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Juri Linkov]

> Lazy highlight doesn't want to try matching in those skip-regions, and
> since it cannot support non-contiguous regions

Adding support for non-contiguous regions to lazy-highlight is trivial -
the same way we did it recently for query-replace.  But to use it we
need a list of boundaries of skipped regions from ispell.  I guess this
shouldn't be too difficult to do using regexps that Alan fixed today
in ispell-skip-region-list.

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Alan Mackenzie]

Hello, Juri.

On Thu, Dec 10, 2015 at 02:04:50AM +0200, Juri Linkov wrote:
> > Lazy highlight doesn't want to try matching in those skip-regions, and
> > since it cannot support non-contiguous regions

> Adding support for non-contiguous regions to lazy-highlight is trivial -
> the same way we did it recently for query-replace.

I don't think I know what you mean, here.  I don't see it as being at
all trivial, but maybe I'm missing something.
isearch-lazy-highlight-new-loop will need to get a list of regions to
skip, and pass this on to i-l-h-update which will need to step gingerly
through the allowed regions.

> But to use it we need a list of boundaries of skipped regions from
> ispell.  I guess this shouldn't be too difficult to do using regexps
> that Alan fixed today in ispell-skip-region-list.

We should be able to take the list of boundaries from the bit of code
which searches for them in the first place.  Maybe.  Somehow.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Juri Linkov]

>> > Lazy highlight doesn't want to try matching in those skip-regions, and
>> > since it cannot support non-contiguous regions
>
>> Adding support for non-contiguous regions to lazy-highlight is trivial -
>> the same way we did it recently for query-replace.
>
> I don't think I know what you mean, here.  I don't see it as being at
> all trivial, but maybe I'm missing something.
> isearch-lazy-highlight-new-loop will need to get a list of regions to
> skip, and pass this on to i-l-h-update which will need to step gingerly
> through the allowed regions.

It's trivial to use a list of regions in lazy-highlight to skip them.
We can do this by adding a new variable isearch-lazy-highlight-regions,
like you recently added isearch-lazy-highlight-window-groups.

>> But to use it we need a list of boundaries of skipped regions from
>> ispell.  I guess this shouldn't be too difficult to do using regexps
>> that Alan fixed today in ispell-skip-region-list.
>
> We should be able to take the list of boundaries from the bit of code
> which searches for them in the first place.  Maybe.  Somehow.

It's not trivial to take the list of boundaries from ispell-skip-region-list.
So the obstacle is in ispell.

bug#22097: Ispell: lazy highlighting doesn't work properly.

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> Here is the current version of my patch for this change.  The comments
> and doc strings in it probably are of too low quality to be committed.

It looks like a version of the patch was applied a few days later, and
I'm not able to reproduce the reported bug in Emacs 28.

There was some discussion afterwards about other ways of doing this, but
as the reported bug here was fixed, I'm closing this bug report now.

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