bug#35202: 27.0.50; Info-quoted false positives and false negatives

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

Hello.  I've noticed that the regexp of Info-mode-font-lock-keywords
throws some false negatives and positives.

In latest Emacs, do these steps to see the bug.
1) Start Emacs: emacs -Q
2) Go to the Emacs Manual: C-h r
3) Go to the Basic node: m Basic RET
4) Go to the Inserting Text section: 1
5) Search for "which is": C-s which is RET
6) Type: M-x describe-text-properties
Notice that face is Info-quoted, even though point is not in a quote.

This is a false positive.  Under the same section, there's another false
positive: "To see which characters have " is quoted too.

Now to show a false negative, in the same Emacs Manual:
Search for this string C-s ‘’’.  That is: C-x 8 [ followed by C-x 8 ]
followed by C-x 8 ].
The search should get you to the Text Display section.  Move point to
the middle ’, the one that is being quoted, and again type:
M-x describe-text-properties.  It doesn't have the face Info-quoted,
even though it is a quote.

I hope I've described well enough the issue.  Proposed patch to follow.

Best regards,
Mauro.

In GNU Emacs 27.0.50 (build 3, i686-pc-linux-gnu, GTK+ Version 3.18.9)
 of 2019-04-08 built on the-blackbeard
Repository revision: 36dc39bfbf1a307769bd62dbe1311a1935737b51
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Ubuntu 16.04.6 LTS

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Mark saved where search started
Type C-x 1 to delete the help window.
Mark saved where search started
Type C-x 1 to delete the help window.

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

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

Major mode: Info

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
  buffer-read-only: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
mml-sec password-cache epa derived epg epg-config gnus-util rmail
rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils iso-transl pp
wid-edit descr-text help-mode cl-loaddefs cl-lib misearch multi-isearch
info easymenu elec-pair mule-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 menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors 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 composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray 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 threads dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 8 76132 6951)
 (symbols 24 6685 1)
 (strings 16 20842 1712)
 (string-bytes 1 613744)
 (vectors 8 10074)
 (vector-slots 4 126132 14196)
 (floats 8 23 19)
 (intervals 28 3882 0)
 (buffers 564 13)
 (heap 1024 8510 811))

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

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

Tags: patch

Please, review this patch.

Best regards,
Mauro.

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

[-- Attachment #2: 0001-Avoid-false-positives-and-false-negatives-of-Info-qu.patch --]
[-- Type: text/x-patch, Size: 985 bytes --]

From 97f666e21e8f2837369ac50efeb31ddb5e07c628 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Mon, 8 Apr 2019 20:24:32 -0300
Subject: [PATCH] Avoid false positives and false negatives of Info-quoted face

* lisp/info.el (Info-mode-font-lock-keywords): Modify the regexp, for
matching single quotes of opening single quote and closing single
quote, and avoid matching text followed by a curly quote when it is
not quoting. (Bug#35202)
---
 lisp/info.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/info.el b/lisp/info.el
index f3b413a..268ecd8 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -4269,7 +4269,7 @@ Info-quoted
 ;; the former can be done much more reliably, i.e. without risking
 ;; false positives.
 (defvar Info-mode-font-lock-keywords
-  '(("‘\\([^’]*\\)’" (1 'Info-quoted))))
+  '(("‘\\([‘’]?\\|[^‘’]*\\)’" (1 'Info-quoted))))
 
 ;; Autoload cookie needed by desktop.el
 ;;;###autoload
-- 
2.7.4

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Mon, 8 Apr 2019 20:30:19 -0300
> 
> diff --git a/lisp/info.el b/lisp/info.el
> index f3b413a..268ecd8 100644
> --- a/lisp/info.el
> +++ b/lisp/info.el
> @@ -4269,7 +4269,7 @@ Info-quoted
>  ;; the former can be done much more reliably, i.e. without risking
>  ;; false positives.
>  (defvar Info-mode-font-lock-keywords
> -  '(("‘\\([^’]*\\)’" (1 'Info-quoted))))
> +  '(("‘\\([‘’]?\\|[^‘’]*\\)’" (1 'Info-quoted))))

Thanks, but I'm worried how we will be able to make sure this change
doesn't break display in some other places.  Would it make sense to
search all our Info files for the new regexp, and examine the display
of all the places that match?  Or did you already do that?

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Noam Postavsky]

Mauro Aranda <maurooaranda@gmail.com> writes:

> -  '(("‘\\([^’]*\\)’" (1 'Info-quoted))))
> +  '(("‘\\([‘’]?\\|[^‘’]*\\)’" (1 'Info-quoted))))

Just a minor nitcpick, both the alternives match the empty string, I
would rather write it to avoid overlap:

        "‘\\([‘’]\\|[^‘’]*\\)’"

or

        "‘\\([‘’]?\\|[^‘’]+\\)’"

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

> Thanks, but I'm worried how we will be able to make sure this change
> doesn't break display in some other places.  Would it make sense to
> search all our Info files for the new regexp, and examine the display
> of all the places that match?  Or did you already do that?
> .

Hello Eli.

I must admit, I didn't check thoroughly, so I can't be sure it doesn't
break the font-locking of quotes in other places.

I have come up with the following way of testing a new regexp:
Step 1: In Info files that come with Emacs, search the original
regexp and the new regexp for all the matches.  Compare both results,
discarding the insersecting values (IOW, discard the matches for which a
new regexp wouldn't introduce new behavior).

Step 2: Based on the previous, analyze each new behavior, to see if a
new regexp does the right thing, or not.

Step 3: Accept, adjust or discard the regexp, accordingly.  If not
accepted, go back to Step 1 for a new iteration.

Provided the final regexp doesn't break display in other places (and
of course, improves the current behavior), would the procedure
I've just described be enough to trust a change?

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

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

> Just a minor nitcpick, both the alternives match the empty string, I
> would rather write it to avoid overlap:
>
>         "‘\\([‘’]\\|[^‘’]*\\)’"
>
> or
>
>         "‘\\([‘’]?\\|[^‘’]+\\)’"

Hello Noam.

Thanks for the correction.  I'll adjust the regexp accordingly.

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

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Tue, 9 Apr 2019 14:40:48 -0300
> Cc: 35202@debbugs.gnu.org
> 
> I have come up with the following way of testing a new regexp:
> Step 1: In Info files that come with Emacs, search the original
> regexp and the new regexp for all the matches.  Compare both results,
> discarding the insersecting values (IOW, discard the matches for which a
> new regexp wouldn't introduce new behavior).
> 
> Step 2: Based on the previous, analyze each new behavior, to see if a
> new regexp does the right thing, or not.
> 
> Step 3: Accept, adjust or discard the regexp, accordingly.  If not
> accepted, go back to Step 1 for a new iteration.
> 
> Provided the final regexp doesn't break display in other places (and
> of course, improves the current behavior), would the procedure 
> I've just described be enough to trust a change?

Sounds good to me, thanks.

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

Hello.

I'll explain shortly how I implemented the test:
I created a list of info files that get built when building emacs, and
for each one, I called (info info-filename) to visit it [1].  Then, searched
all the file for the old regexp, storing the values in a list, each
element being a list: '((match-beginning 1) (1- (match-end 1))).  Then,
something similar for the new regexp.  For the change of regexp to have
effect, I added a hook to Info-mode-hook, that basically does this:

(setcar (car Info-mode-font-lock-keywords) current-re)
(setq-local font-lock-defaults '(Info-mode-font-lock-keywords t t)))

Finally, compared both lists (namely old-matches and new-matches) with
cl-set-exclusive-or, sorted it (for better comparison) and wrote a file
.mismatches-filename, for each info file.


Now for the results:
The files that presented mismatches are the following:
emacs (as expected, hence the bug report), calc, idlwave, mh-e, org,
sc.

To navigate to the points, for examination, I recommend widening the
info buffer and then goto-char.

* Emacs:
1)
Old match: (92506 92541)
New match: (92536 92541)

2)
Old match: (92823 92860)
New match: (92856 92860)

This is correct, and achieved by the second option of the regexp I
proposed.

3)
Old match: (183951 183977)
New match: (183952 183977)

This is a little odd, since it is a quote inside a quote.  The new
regexp matches the inner quote, while the old quote quotes the ‘
starting the inner quote too.  There's no big difference, IMO.

4)
Old match: (313527 313526)
New match: (313527 313527)

5)
Old match: (313905 313904)
New match: (313905 313905)

4) and 5) are the same.  This was part of the original bug report, so as
expected, the new regexp handles this case just right.

6)
Old match: (652524 652542)
New match: (652536 652542)

Similar to 1) and 2).

7)
Old match: (767119 767124)
New match: (767123 767124)

This one is tricky.  It is a quote that contains ‘ and ’, but it is not
a nested quote.  Tweaking the regexp to match nested quotes would do the
right thing, but by sheer luck.

8)
Old match: (768216 768225)
New match: (768219 768225)

See 1) and 2).

* Calc:
9)
Old match: (493087 493098)
New match: (493088 493098)

This is odd, and might be the calc.texi file that is wrong (I'm not
sure, but the "`" in calc.texi looks suspicious).  Still, the new
behavior doesn't break display with this one, IMO.

10) Something extra I noted  in the Appendix E Calc Summary.
Both regexp fails at (1386635 1404639).  I found this a hard one, and I
can't think of a way to solve it.

* Idlwave:
11)
Old match: (93451 93514)
New match: (93496 93514)

Both regexp are wrong in this table.  This is similar to 10).  Not an
easy one to solve, but the new regexp at least behaves a little better
in the line with (‘idlwave-find-module’), IMO.

* MH-E:
12) This one is a group of similar mismatches:
Old ones:
(168432 168456)
(168585 168611)
(168755 168774)

New ones:
(168456 168456)
(168611 168611)
(168774 168774)

The old regexp quotes inconsistently, while the proposed one quotes only
the ‘+’, ‘-’ and the ‘r’.  I think it could be solved by tweaking the
proposed regexp, to match the outer quote of a nested quote.

* Org:
13) Go to the table at 685320.  The problem with the mismatches is
similar to 11), and both regexp get it wrong.

* SC:
14)
Old matches:
(9549 9550)
(9768 9769)

New matches:
(9550 9550)
(9769 9769)

I'm not sure if the double quoting of > (as in ‘‘>’’) is intended.  I
don't think so, but I can't be sure.  Still, the new regexp behaves
better, by quoting only the >, while the current one is inconsistent and
looks odd.


To sum it up:
* Not sure if the tables in Idlwave and Org could be changed.  If yes,
then the problems with these files will go away.

* If 9) and 14) can be solved by modifying the .texi file, then either
regexp will do.

* The regexp could be tweaked to match outer quotes, when quotes are
nested.  This is necessary to do the right thing in mh-e file, for
example.

* Overall, I think it is an improvement.  It doesn't break display, it is
more
accurate, and wherever it fails, the current regexp fails too. But of
course,
I'm biased, since I'm the one proposing it.

[1] Files I checked:
ada-mode, auth, autotype, bovine, calc, ccmode, cl, dbus, dired-x,
ebrowse, ede, ediff, edt, efaq, efaq-w32, eieio, elisp, eintr, emacs,
emacs-gnutls, emacs-mime, epa, erc, ert, eshell, eudc, eww, flymake,
forms, gnus, htmlfontify, idlwave, ido, info, mairix-el, message, mh-e,
nesticker, nxml-mode, octave-mode, org, pcl-cvs, pgg, ricrc, reftex,
remember, sasl, sc, semantic, ses, sieve, smtpmail, speedbar, srecode,
todo-mode, tramp, url, vhdl-mode, vip, viper, widget, wisent, woman.

For extra points, I checked some external files I happen to have
installed:
libc, bison, wget.

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

[-- Attachment #2: 0001-Avoid-false-positives-and-false-negatives-of-Info-qu.patch --]
[-- Type: text/x-patch, Size: 1103 bytes --]

From fe255c115680339fcfd59000cfc867abcf6995bd Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Mon, 8 Apr 2019 20:24:32 -0300
Subject: [PATCH] Avoid false positives and false negatives of Info-quoted face

* lisp/info.el (Info-mode-font-lock-keywords): Modify the regexp, for
matching single quotes of opening single quote and closing single
quote, and avoid matching text followed by a curly quote when it is
not quoting. (Bug#35202)
---
 lisp/info.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/info.el b/lisp/info.el
index f3b413a..9a8c82e 100644
--- a/lisp/info.el
+++ b/lisp/info.el
@@ -4268,8 +4268,9 @@ Info-quoted
 ;; We deliberately fontify only ‘..’ quoting, and not `..', because
 ;; the former can be done much more reliably, i.e. without risking
 ;; false positives.
+;; FIXME: It doesn't handle nested quotes.
 (defvar Info-mode-font-lock-keywords
-  '(("‘\\([^’]*\\)’" (1 'Info-quoted))))
+  '(("‘\\([‘’]\\|[^‘’]*\\)’" (1 'Info-quoted))))
 
 ;; Autoload cookie needed by desktop.el
 ;;;###autoload
-- 
2.7.4

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Wed, 10 Apr 2019 21:19:54 -0300
> Cc: 35202@debbugs.gnu.org
> 
> Now for the results:
> The files that presented mismatches are the following:
> emacs (as expected, hence the bug report), calc, idlwave, mh-e, org,
> sc.
> 
> To navigate to the points, for examination, I recommend widening the
> info buffer and then goto-char.

Thanks.

I think you used some old version of the Info files, because your
buffer positions sometimes don't match at all with what I see, so I
cannot be sure what to look at.  Could you please specify, for each
spot you analyzed, the node name and the text surrounding the regexp
match?  I'd like to go over all the places you found, but I need a
better guide than just buffer position, because that depends on
various factors out of my control, like the version of Texinfo used to
generate the Info files.

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

> cannot be sure what to look at.  Could you please specify, for each
> spot you analyzed, the node name and the text surrounding the regexp
> match?  I'd like to go over all the places you found, but I need a

Sure Eli.  Sorry for not thinking about that before.

In the following, navigate to the mentioned node of the respective
manual, and either search for the string in "Search for", or just search
for the string in "Old match".  The string in "Search for" is the text
just before the spot analyzed.

* Emacs (‘C-h r’):
1) Node: Inserting Text (‘m Inserting Text’)
Search for: For example, ‘C-x 8 [’ inserts
Old match: which is Unicode code-point ‘U+2018
New match: U+2018

2) Node: Inserting Text (‘m Inserting Text’)
Search for: C-x 8 [’ and inserts
Old match: .  To see which characters have ‘C-x 8
New match: C-x 8

The new regexp is right, the old one is wrong.

3) Node: Setting Mark (‘m Setting Mark’)
Search for: (‘mouse-save-then-kill’).
Old match: ‘Shifted cursor motion keys
New match: Shifted cursor motion keys

Nested quotes: The new regexp matches all of the inner quote,
while the old one leaves the first ’ unquoted.

4) and 5) Node: Text Display (‘m Text Display’)
Search for: Emacs tries to determine if the curved quotes
Old match: The empty string in ‘’’.
New match: ’

Similarly, search for: If the curved quotes.
Same situation as before.

The new regexp is right, the old one is wrong (it fails to quote the ’).

6) Node: International Chars (‘m International Chars’)
Search for: a left single quotation mark
Old match: by typing ‘C-x 8 [
New match: C-x 8 [

Similar to 1) and 2).

7) Node: Quotation Marks (‘m Quotation Marks’)
Search for: "whose default value is"
Old match: '(?‘ ?
New match:  ?

Quote characters inside a quote, but not making an inner quote.  Both
regexps get it wrong.

8) Node: Quotation Marks (‘m Quotation Marks’)
Search for: you can type ‘C-x 8 [’ for
Old match: , ‘C-x 8 ]
New match: C-x 8 ]

Similar to 1) and 2).

* Calc (‘C-h i m Calc RET’)
9) Node: Yacas Language Mode (‘7 8 5’)
Search for: in Yacas mode, and
Old match: ‘arcsin(x^2)
New match: arcsin(x^2)

I find this one a little weird.  In the calc.texi file, I navigated to
this spot, by searching for "arcsin(x^2)".  There is a suspicious "`"
before @samp.

10) Node: Summary (‘m Summary RET’)
Search for: calc-grab-sum-across C-x *

Sorry, I don't know how to specify more this one (the text matched is
very long, for both regexps).  Notice that the text matched begins in
" editing" and ends up in the row that has a "@".
Both regexps fail.

11) Node: Summary (‘m Summary RET’)
‘m Summary RET’
Search for: cancel if/loop command Z

Same as before.  The quoting can be seen in "calc-kbd-push Z".
Both regexps fail.


* MH-E (‘C-h i m MH-E’)
12) Node: Speedbar (‘m Speedbar’)
Search for: ‘mh-speed-view’).
Old match: Expand Nested Folders (‘+
New match: +

Similar situation for the next two items:
Old match: Contract Nested Folders (‘-
New match: -

Old match: Refresh Speedbar (‘r
New match: r

While the old regexp quotes inconsistently, the old one just quotes the
shortcut.  A regexp that matches nested quotes (and quotes the outer
ones) could do the trick (though I believe it could break cases like
1) and 2), for example).


* Org (‘C-h i m Org RET’)
13) Node: TTY keys (‘m Miscellaneous RET 9)
Search for: ‘S-<RIGHT>’

As with the Calc table, it is not easy to write this one.
Notice that there are keybindings that take two rows:
C-c <RIGHT>
C-c C-x <LEFT>
C-c C-x <RIGHT>
The old regexp matches up until the first ’, leaving the end of the
keybinding unquoted.
The new one, just matches the space between ‘ and ’ (under the Speed key
column of the table), leaving the keybinding unquoted.

Similar to 11), and both regexps get it wrong.

* SC (‘C-h i m SC RET’)
14) Node: Citations (‘m Citations’)
Search for: result in a nesting of the
Old match: ‘>
New match: >

Similarly,
search for: and multiple nesting of
Old match: ‘>
New match: >

Again a case of nested quotes, though I think the nested quote is not
necessary.  The old regexp quotes inconsistently, making it look
uglier.  The new one just quote the important character, >.


That is all.  I built the info files from the latest master I could get,
just in case.  That is, commit: c2c9ef2c1e509f1a01ffaefd7475004854162a10
I think it is much easier to navigate to the problematic spots now, but
please let me know if I missed something again.

Best regards,
Mauro.

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

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Sat, 13 Apr 2019 10:26:19 -0300
> Cc: 35202@debbugs.gnu.org
> 
> > cannot be sure what to look at.  Could you please specify, for each
> > spot you analyzed, the node name and the text surrounding the regexp
> > match?  I'd like to go over all the places you found, but I need a
> 
> Sure Eli.  Sorry for not thinking about that before.
> 
> In the following, navigate to the mentioned node of the respective
> manual, and either search for the string in "Search for", or just search
> for the string in "Old match".  The string in "Search for" is the text
> just before the spot analyzed.

Thanks.  I've proofread all these places, and installed your patch.  I
also fixed a few typos that your analysis uncovered.

With that, I'm closing this bug.  Thanks a lot for your work on this
tricky issue.

bug#35202: 27.0.50; Info-quoted false positives and false negatives

[Mauro Aranda]

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

> Thanks.  I've proofread all these places, and installed your patch.  I
> also fixed a few typos that your analysis uncovered.
>
> With that, I'm closing this bug.  Thanks a lot for your work on this
> tricky issue.

That's great.  You're welcome, I'm happy to be of any help.

Best regards,
Mauro.

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