bug#19892: 25.0.50; hideshow: hs-hide-all-non-comment-function example infloop

bug#19892: 25.0.50; hideshow: hs-hide-all-non-comment-function example infloop

[Michael Heerdegen]

Hello,

  (CCing the authors of hideshow specified in the file header)

in the header of hideshow.el, we have the following paragraph:

--8<---------------cut here---------------start------------->8---
;; Some languages (e.g., Java) are deeply nested, so the normal behavior
;; of `hs-hide-all' (hiding all but top-level blocks) results in very
;; little information shown, which is not very useful.  You can use the
;; variable `hs-hide-all-non-comment-function' to implement your idea of
;; what is more useful.  For example, the following code shows the next
;; nested level in addition to the top-level:
;;
;;   (defun ttn-hs-hide-level-1 ()
;;     (hs-hide-level 1)
;;     (forward-sexp 1))
;;   (setq hs-hide-all-non-comment-function 'ttn-hs-hide-level-1)
--8<---------------cut here---------------end--------------->8---

But this doesn't always work.  For example, eval the above example in
emacs -Q, open "files.el", M-x hs-minor-mode, M-x hs-hide-all.  You get
an infloop.

AFAICT the definition should be (or at least it works with that)

(defun ttn-hs-hide-level-1 ()
  (when (hs-looking-at-block-start-p)
    (hs-hide-level 1))
  (forward-sexp 1))


Secondly, there is this comment in `hs-hide-all' which confuses me a bit:

;; Go to end of matched data to prevent from getting stuck
;; with an endless loop.

Which match data is meant there?  It is either match data from before
hiding the block - then it should be documented that
`hs-hide-all-non-comment-function' must not change match data, I guess,
or the call should be wrapped into `save-match-data'.  Or it is even the
case that `hs-hide-all-non-comment-function' must set the match data
(how?), which then should probably be documented.


Thanks,

Michael.






In GNU Emacs 25.0.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.14.5)
 of 2015-02-15 on drachen
Repository revision: ba5bc0ee7c81f2122072bee162fcf1dbd8b2a8f2
Windowing system distributor `The X.Org Foundation', version 11.0.11602901
System Description:	Debian GNU/Linux 8.0 (jessie)

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND DBUS GSETTINGS NOTIFY
LIBXML2 FREETYPE XFT ZLIB

Important settings:
  value of $LC_ALL: de_DE.utf8
  value of $LC_COLLATE: C
  value of $LC_TIME: C
  value of $LANG: de_DE.utf8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  magit-auto-revert-mode: t
  rainbow-delimiters-mode: t
  paredit-mode: t
  on-screen-global-mode: t
  ml-scale-mode: t
  interaction-log-mode: t
  highlight-defined-mode: t
  helm-descbinds-mode: t
  helm-mode: t
  shell-dirtrack-mode: t
  helm-occur-match-plugin-mode: t
  helm-match-plugin-mode: t
  helm-autoresize-mode: t
  global-diff-hl-mode: t
  diff-hl-mode: t
  diff-auto-refine-mode: t
  recentf-mode: t
  which-function-mode: t
  winner-mode: t
  show-paren-mode: t
  auto-image-file-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  mouse-wheel-mode: t
  prettify-symbols-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
  column-number-mode: t
  line-number-mode: t
  hs-minor-mode: t

bug#19892: 25.0.50; hideshow: hs-hide-all-non-comment-function example infloop

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> in the header of hideshow.el, we have the following paragraph:

[...]

> ;;   (defun ttn-hs-hide-level-1 ()
> ;;     (hs-hide-level 1)
> ;;     (forward-sexp 1))
> ;;   (setq hs-hide-all-non-comment-function 'ttn-hs-hide-level-1)
>
> But this doesn't always work.  For example, eval the above example in
> emacs -Q, open "files.el", M-x hs-minor-mode, M-x hs-hide-all.  You get
> an infloop.
>
> AFAICT the definition should be (or at least it works with that)
>
> (defun ttn-hs-hide-level-1 ()
>   (when (hs-looking-at-block-start-p)
>     (hs-hide-level 1))
>   (forward-sexp 1))

I've confirmed that the example still infloops in Emacs 27, and that
your example doesn't, so I've committed it to the Emacs trunk.

> Secondly, there is this comment in `hs-hide-all' which confuses me a bit:
>
> ;; Go to end of matched data to prevent from getting stuck
> ;; with an endless loop.
>
> Which match data is meant there?  It is either match data from before
> hiding the block - then it should be documented that
> `hs-hide-all-non-comment-function' must not change match data, I guess,
> or the call should be wrapped into `save-match-data'.  Or it is even the
> case that `hs-hide-all-non-comment-function' must set the match data
> (how?), which then should probably be documented.

Here's the code:

             (progn
               (goto-char (match-beginning 1))
	       (unless (if hs-hide-all-non-comment-function
			   (funcall hs-hide-all-non-comment-function)
			 (hs-hide-block-at-point t))
		 ;; Go to end of matched data to prevent from getting stuck
		 ;; with an endless loop.
		 (goto-char (match-end 0))))

`hs-hide-block-at-point' calls a lot of functions that change match
data, so presumably `hs-hide-all-non-comment-function' is allowed, too.
But this all seems nonsensical -- what `match-end' points to here seems
pretty random.  So it looks like a bug to me.

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

bug#19892: 25.0.50; hideshow: hs-hide-all-non-comment-function example infloop

[Lars Ingebrigtsen]

This was introduced (by me applying the following patch) seven years
ago.

Hm...  Oh, I see what the brittle logic here really is.  The code (by
default) calls this function:

(defun hs-hide-block-at-point (&optional end comment-reg)
[...]
    (when (hs-looking-at-block-start-p)

And:

(defun hs-looking-at-block-start-p ()
  "Return non-nil if the point is at the block start."
  (and (looking-at hs-block-start-regexp)
       (save-match-data (not (nth 8 (syntax-ppss))))))

So if the match is successful, but whatever (nth 8 (syntax-ppss)) means
doesn't, then we go to the end of that match data.

So it kinda works...  But not if `hs-hide-all-non-comment-function' is
set.  I'll try to rework this in a more logical fashion.

commit 43956923c00921499e0582f5da4cd739bc005240
Author: Sébastien Gross <seb@chezwam.org>
Date:   Wed Apr 11 01:34:25 2012 +0200

    (hs-hide-all): Don't infloop on comments that start in the middle of the line.
    
    Fixes: debbugs:10496

diff --git a/lisp/progmodes/hideshow.el b/lisp/progmodes/hideshow.el
--- a/lisp/progmodes/hideshow.el
+++ b/lisp/progmodes/hideshow.el
@@ -804,7 +804,10 @@
          (if (match-beginning 1)
-             ;; we have found a block beginning
+             ;; We have found a block beginning.
              (progn
                (goto-char (match-beginning 1))
-               (if hs-hide-all-non-comment-function
-                   (funcall hs-hide-all-non-comment-function)
-                 (hs-hide-block-at-point t)))
+	       (unless (if hs-hide-all-non-comment-function
+			   (funcall hs-hide-all-non-comment-function)
+			 (hs-hide-block-at-point t))
+		 ;; Go to end of matched data to prevent from getting stuck
+		 ;; with an endless loop.
+		 (goto-char (match-end 0))))



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