bug#20146: font-lock-extend-jit-lock-region-after-change: results are discarded instead of being returned.

[Alan Mackenzie]

Hello, Stefan.

On Sat, Mar 21, 2015 at 06:30:26PM -0400, Stefan Monnier wrote:
> >> The major mode sets font-lock-extend-region-function and this functions'
> >> result should be (and is) respected by the rest of font-lock.
> > If only, but this is simply not the case at the moment.
> > jit-lock-fontify-now has a hard-coded extension to whole lines,
> > regardless of the contents of font-lock-extend-region-functions.

> There's no relationship between the two.  The bounds that
> font-lock-fontify-region [gets] are not under the major mode's control.

Yes they are.  They can be set by
font-lock-extend-after-change-region-function, as described in the elisp
manual.

> What is under the major mode's control is how those bounds are extended
> by font-lock-extend-region-functions.

But by that time, jit-lock has already played fast and loose with the
bounds supplied by the major mode.  It is too late by that time for
anything in font-lock-fontify-region to repair the damage.

> > This is surely a bug.

> Nope.

Yes.  I think you've said, indeed quite often, that font-lock has the
same behaviour regardless of whether jit-lock is enabled or not.  Here,
there is a marked difference of behaviour: different regions are
provided to font-lock-fontify-region in the two cases.

> > As a better idea, why not have jit-lock-fontify-now use
> > f-l-extend-region-functions, and then pass the original `start' and
> > `next' to f-l-fontify-region.

> No, you have that backwards: it's f-l-fontify-region which runs
> f-l-extend-region-functions, specifically so that you don't need to care
> about what jit-lock (and other callers of f-l-fontify-region) might do
> (you just need to make sure that f-l-fontify-region works correctly for
> *any* bounds).

I need to care a great deal about what jit-lock does, for the reasons in
this thread.  One way out of the current problem might involve testing
font-lock-support-mode in the CC Mode code.  This would not be ideal.

Looking more critically at jit-lock-fontify-now, there is simply no need
for it to expand the region to whole lines.  The only things it ever does
itself with (start next) are
(i) Update jit-lock-context-unfontify-pos;
(ii) Set fontified text properties;
(iii) Call f-l-fontify-region (and other functions on
  jit-lock-functions).

Not extending (start next) to whole lines
(i) wouldn't make any difference, or if it would, it could be handled at
  the place where jit-lock-context-unfontify-pos is set;
(ii) wouldn't make any difference at all;
(iii) wouldn't make any difference to f-l-fontify-region, which itself
  does all the requisite region extending.  The other functions
  (bug-reference-fontify, glasses-change, goto-address-fontify-region)
  may need a little adjustment, but probably not.

As a bonus, not extending (start next) would completely prevent the
condition that triggers (via a timer) jit-lock-force-redisplay, so we
could remove that part of the code and j-l-f-redisplay itself.  (It is
true that buffer positions before start could have been refontified, but
that is also true of the existing code).

Likewise, there is then no longer any purpose in extending to whole
lines in font-lock-extend-jit-lock-region-after-change, so we can remove
that bit, too.

All in all, a massive simplification, and it allows #19669 to be fixed
easily, at least for Emacs 25.1.

Abstractly seen, splitting the action of extension between two modules
is poor design, leading to problems.  Part of this extending MUST be
done in f-l-fontify-region, so it makes sense to do it only there.

> >> But callers of font-lock-fontify-region (such as
> >> font-lock-after-change-function, or jit-lock) can choose *any* bounds
> >> they feel like and font-lock-fontify-region should behave correctly.
> > This seems to be true.  When I (setq font-lock-support-mode nil), things
> > seem to behave properly.  It seems a poor workaround, though.

> I don't see how what you say relates to what you quoted.

Sorry, it was a bit opaque.  I think I was saying that disabling
jit-lock enables the major mode to chose the bounds given to
f-l-fontify-region (via f-l-extend-after-change-r-f), and that f-l-f-r
does indeed behave correctly w.r.t. whatever bounds it gets.  But
disabling jit-lock would be a poor workaround to the problem.

So, as an opener, I propose the following improvement:


diff --git a/lisp/font-lock.el b/lisp/font-lock.el
index 1838a0f..c62945d 100644
--- a/lisp/font-lock.el
+++ b/lisp/font-lock.el
@@ -1316,20 +1316,6 @@ This function does 2 things:
                 ;; the line was deleted.  Or a \n was inserted in the middle
                 ;; of a line.
                 (1+ end))))
-      ;; Finally, pre-enlarge the region to a whole number of lines, to try
-      ;; and anticipate what font-lock-default-fontify-region will do, so as to
-      ;; avoid double-redisplay.
-      ;; We could just run `font-lock-extend-region-functions', but since
-      ;; the only purpose is to avoid the double-redisplay, we prefer to
-      ;; do here only the part that is cheap and most likely to be useful.
-      (when (memq 'font-lock-extend-region-wholelines
-                  font-lock-extend-region-functions)
-        (goto-char beg)
-        (setq beg (min jit-lock-start (line-beginning-position)))
-        (goto-char end)
-        (setq end
-              (max jit-lock-end
-                   (if (bolp) (point) (line-beginning-position 2)))))
       (setq jit-lock-start beg
            jit-lock-end end))))
 
diff --git a/lisp/jit-lock.el b/lisp/jit-lock.el
index 788646c..933bbe3 100644
--- a/lisp/jit-lock.el
+++ b/lisp/jit-lock.el
@@ -366,7 +366,7 @@ Defaults to the whole buffer.  END can be out of bounds."
      ;; from the end of a buffer to its start, can do repeated
      ;; `parse-partial-sexp' starting from `point-min', which can
      ;; take a long time in a large buffer.
-     (let ((orig-start start) next)
+     (let (next)
        (save-match-data
         ;; Fontify chunks beginning at START.  The end of a
         ;; chunk is either `end', or the start of a region
@@ -376,15 +376,6 @@ Defaults to the whole buffer.  END can be out of bounds."
           (setq next (or (text-property-any start end 'fontified t)
                          end))
 
-          ;; Decide which range of text should be fontified.
-          ;; The problem is that START and NEXT may be in the
-          ;; middle of something matched by a font-lock regexp.
-          ;; Until someone has a better idea, let's start
-          ;; at the start of the line containing START and
-          ;; stop at the start of the line following NEXT.
-          (goto-char next)  (setq next (line-beginning-position 2))
-          (goto-char start) (setq start (line-beginning-position))
-
            ;; Make sure the contextual refontification doesn't re-refontify
            ;; what's already been refontified.
            (when (and jit-lock-context-unfontify-pos
@@ -409,35 +400,9 @@ Defaults to the whole buffer.  END can be out of bounds."
             (quit (put-text-property start next 'fontified nil)
                   (funcall 'signal (car err) (cdr err))))
 
-           ;; The redisplay engine has already rendered the buffer up-to
-           ;; `orig-start' and won't notice if the above jit-lock-functions
-           ;; changed the appearance of any part of the buffer prior
-           ;; to that.  So if `start' is before `orig-start', we need to
-           ;; cause a new redisplay cycle after this one so that any changes
-           ;; are properly reflected on screen.
-           ;; To make such repeated redisplay happen less often, we can
-           ;; eagerly extend the refontified region with
-           ;; jit-lock-after-change-extend-region-functions.
-           (when (< start orig-start)
-            (run-with-timer 0 nil #'jit-lock-force-redisplay
-                             (copy-marker start) (copy-marker orig-start)))
-
           ;; Find the start of the next chunk, if any.
           (setq start (text-property-any next end 'fontified nil))))))))
 
-(defun jit-lock-force-redisplay (start end)
-  "Force the display engine to re-render START's buffer from START to END.
-This applies to the buffer associated with marker START."
-  (when (marker-buffer start)
-    (with-current-buffer (marker-buffer start)
-      (with-buffer-prepared-for-jit-lock
-       (when (> end (point-max))
-         (setq end (point-max) start (min start end)))
-       (when (< start (point-min))
-         (setq start (point-min) end (max start end)))
-       ;; Don't cause refontification (it's already been done), but just do
-       ;; some random buffer change, so as to force redisplay.
-       (put-text-property start end 'fontified t)))))
 
 ;;; Stealth fontification.
 

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).