bug#56848: CC Mode fontification bug

bug#56848: CC Mode fontification bug

[Gregory Heytings]

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


Recipe:

emacs -Q
C-x C-f src/xdisp.c RET
M-g c 28 RET
;; take note of the word there: "window"
M-: (get-char-property 28 'fontified) RET
;; observe that this returns t
M-g g 800 RET
C-v
M-: (get-char-property 28 'fontified) RET
;; observe that this returns nil, because "struct window" is now visible

This is a BUG.

When font locking has put a fontified property and one of the 
font-lock-*-faces on characters in the buffer, a mode should not undo that 
unless it has a very good reason to do so.  Otherwise scrolling again 
through an already fontified buffer calls fontification functions again 
without reason.

Patch attached.

With the patch, scrolling again through an already fontified buffer is 
~25% faster.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Be-conservative-with-occurrences-of-newly-found-types.patch --]
[-- Type: text/x-diff; name=Be-conservative-with-occurrences-of-newly-found-types.patch, Size: 1803 bytes --]

From 4f017e72850a19fa6bfba33663652b8c638bb825 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sun, 31 Jul 2022 00:04:07 +0000
Subject: [PATCH] Be conservative with occurrences of newly found types.

* lisp/progmodes/cc-fonts.el (c-fontify-new-found-type): Remove the
fontified property on occurrences of newly found types only when
they do not yet have a face property.
---
 lisp/progmodes/cc-fonts.el | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/lisp/progmodes/cc-fonts.el b/lisp/progmodes/cc-fonts.el
index 625010b04b..75936574a1 100644
--- a/lisp/progmodes/cc-fonts.el
+++ b/lisp/progmodes/cc-fonts.el
@@ -2284,16 +2284,17 @@ c-fontify-new-found-type
 	  (widen)
 	  (goto-char (point-min))
 	  (while (re-search-forward target-re nil t)
-	    (put-text-property (match-beginning 0) (match-end 0)
-			       'fontified nil)
-	    (dolist (win-boundary window-boundaries)
-	      (when (and (< (match-beginning 0) (cdr win-boundary))
-			 (> (match-end 0) (car win-boundary))
-			 (not c-re-redisplay-timer))
-		(setq c-re-redisplay-timer
-		      (run-with-timer 0 nil #'c-force-redisplay
-				      (current-buffer)
-				      (match-beginning 0) (match-end 0)))))))))))
+	    (unless (get-char-property (match-beginning 0) 'face)
+	      (put-text-property (match-beginning 0) (match-end 0)
+				 'fontified nil)
+	      (dolist (win-boundary window-boundaries)
+		(when (and (< (match-beginning 0) (cdr win-boundary))
+			   (> (match-end 0) (car win-boundary))
+			   (not c-re-redisplay-timer))
+		  (setq c-re-redisplay-timer
+			(run-with-timer 0 nil #'c-force-redisplay
+					(current-buffer)
+					(match-beginning 0) (match-end 0))))))))))))
 
 \f
 ;;; C.
-- 
2.35.1

bug#56848: CC Mode fontification bug

[Eli Zaretskii]

> Date: Sun, 31 Jul 2022 00:16:37 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> emacs -Q
> C-x C-f src/xdisp.c RET
> M-g c 28 RET
> ;; take note of the word there: "window"
> M-: (get-char-property 28 'fontified) RET
> ;; observe that this returns t
> M-g g 800 RET
> C-v
> M-: (get-char-property 28 'fontified) RET
> ;; observe that this returns nil, because "struct window" is now visible

As an aside, one should be very careful with trusting the likes of

   M-: (get-char-property 28 'fontified) RET

because entering the minibuffer triggers a rather thorough redisplay
cycle, which could change the 'fontified' property one is trying to
obtain.  Instead, it is advisable to write a simple command that would
do the evaluation, then bind it to a single key, like F5, and invoke
through that key.  Even better, invoke the function from the debugger.

(I'm not saying that the nil above is inaccurate, since the
problematic position is outside the window, I'm just saying one should
be very careful with this stuff.)

FWIW, I can reproduce this on master, but not on the release branch.
I thought this was related to bug#56841, but since that one happens on
emacs-28 as well, it probably is a separate issue.

> When font locking has put a fontified property and one of the 
> font-lock-*-faces on characters in the buffer, a mode should not undo that 
> unless it has a very good reason to do so.  Otherwise scrolling again 
> through an already fontified buffer calls fontification functions again 
> without reason.

The fact that the word 'window' is involved in both cases seems to
ring a bell: isn't there a feature in CC Mode's fontifications whereby
it does something with identifiers whose type it knows about, by going
forward and back into the buffer and "fixing" their fontifications?

Thanks.

bug#56848: CC Mode fontification bug

[Gregory Heytings]

>
> As an aside, one should be very careful with trusting the likes of
>
>   M-: (get-char-property 28 'fontified) RET
>
> because entering the minibuffer triggers a rather thorough redisplay 
> cycle, which could change the 'fontified' property one is trying to 
> obtain.  Instead, it is advisable to write a simple command that would 
> do the evaluation, then bind it to a single key, like F5, and invoke 
> through that key.  Even better, invoke the function from the debugger.
>
> (I'm not saying that the nil above is inaccurate, since the problematic 
> position is outside the window, I'm just saying one should be very 
> careful with this stuff.)
>

Indeed, I tried to make the recipe as simple as possible, but during my 
tests I did use such a function.

>
> The fact that the word 'window' is involved in both cases seems to ring 
> a bell: isn't there a feature in CC Mode's fontifications whereby it 
> does something with identifiers whose type it knows about, by going 
> forward and back into the buffer and "fixing" their fontifications?
>

Yes, that's what the 'c-fontify-new-found-type' modified by the patch 
does.  Currently, when a new type has been found, all occurrences of its 
identifier are "unfontified" with:

(widen)
(goto-char (point-min))
(while (re-search-forward ...) (put-text-property ... 'fontified nil))

When font locking has already put, say, a font-lock-comment-face, or a 
font-lock-function-name-face, or a font-lock-variable-name-face on an 
occurrence of that identifier, there is no reason to undo that.  It is 
only occurrences that have been marked as "fontified" but on which no face 
has been put that should be unfontified.

bug#56848: CC Mode fontification bug

[Gregory Heytings]

Closing this bug.  The proposed patch has been included in commits 
4bd8ad2bc5 and 1cbc22b9c7.

bug#56848: CC Mode fontification bug

[Alan Mackenzie]

reopen 56848
quit

Hello, Gregory.

Thanks for the Cc:!

I'm (trying to) reopen the bug with this post.

On Sat, Jan 14, 2023 at 21:28:24 +0000, Gregory Heytings wrote:

> Closing this bug.  The proposed patch has been included in commits 
> 4bd8ad2bc5 and 1cbc22b9c7.

I wasn't previously aware of this bug.  I'm not at all happy about the
patches you proposed and have applied.  In particular, you say
(2022-07-31):

> When font locking has already put, say, a font-lock-comment-face, or a
> font-lock-function-name-face, or a font-lock-variable-name-face on an
> occurrence of that identifier, there is no reason to undo that.  It is
> only occurrences that have been marked as "fontified" but on which no
> face has been put that should be unfontified.

, without justification.  There are circumstances in which identifiers
with font-lock-type-face need to be refontified with
font-lock-variable-face, and the reverse, amongs others.  It was trying
to optimise in this area which caused some of Po Lu's bug reports over
the last three months.

An example of what can go wrong is, supposing we have just mistyped

    food sausage;

as

    foo sausage;

..  "All" the identifiers "foo" throughout the buffer will get fontified
with type-face.  On returning to the line and correcting "foo" to
"food", these "foo"s need to be refontified.  This actual scenario
occurred in one of Po's bug reports.

I think that after the patches in this bug, the refontification won't
happen.  I've reopened the bug, and I'm asking you to reconsider these
things.

Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#56848: CC Mode fontification bug

[Gregory Heytings]

Hi Alan,

>> Closing this bug.  The proposed patch has been included in commits 
>> 4bd8ad2bc5 and 1cbc22b9c7.
>
> I wasn't previously aware of this bug.  I'm not at all happy about the 
> patches you proposed and have applied.
>

My post was apparently not clear enough, sorry for that: I did not apply 
anything, I closed this bug and recorded in the bug tracker that it was 
fixed by two of your commits.  The current state of the function that the 
patch modified is essentially what was proposed in the patch.