bug#9919: 24.0.91; font-lock broken in dired

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

This regression started with the second delivery of pretest #1,
2011-10-31.  There is no such problem with the first delivery,
2011-10-24.
 
In my setup I use dired+.el, which is here:
http://www.emacswiki.org/emacs/download/dired%2b.el.
 
The relevant code is this:
 
(add-hook 'dired-mode-hook
          '(lambda ()
            (set (make-local-variable 'font-lock-defaults)
             (cons '(dired-font-lock-keywords diredp-font-lock-keywords-1) ; Two
levels.
              (cdr font-lock-defaults)))))
 
And this:

(defvar diredp-font-lock-keywords-1
  (list
   '("^  \\(.+:\\)$" 1 diredp-dir-heading) ; Directory headers
   '("^  wildcard.*$" 0 'default)       ; Override others, e.g. `l' for
`diredp-other-priv'.
   '("^  (No match).*$" 0 'default)     ; Override others, e.g. `t' for
`diredp-other-priv'.
   '("[^ .]\\.\\([^. /]+\\)$" 1 diredp-file-suffix) ; Suffix
   '("\\([^ ]+\\) -> [^ ]+$" 1 diredp-symlink) ; Symbolic links
   ;; 1) Date/time and 2) filename w/o suffix:
   (list dired-move-to-filename-regexp
         (if (or (not (fboundp 'version<)) (version< emacs-version "23.2"))
             (list 1 'diredp-date-time t t)
           (list 2 'diredp-date-time t t)) ; Date/time
         (list "\\(.+\\)$" nil nil (list 0 diredp-file-name 'keep t))) ;
Filename
 
   ;; Files to ignore
   (list (concat "^  \\(.*\\(" (concat (mapconcat 'regexp-quote
                                                  (or (and (boundp
'dired-omit-extensions)
 
dired-omit-extensions)
 
completion-ignored-extensions)
                                                  "[*]?\\|")
                                       "[*]?")        ; Allow for executable
flag (*).
                 "\\|\\.\\(g?z\\|Z\\)[*]?\\)\\)$") ; Compressed.
         1 diredp-ignored-file-name t)
   '("[^ .]\\.\\([bg]?[zZ]2?\\)[*]?$" 1 diredp-compressed-file-suffix t) ;
Compressed (*.z)
   '("\\([*]\\)$" 1 diredp-executable-tag t) ; Executable (*)
   ;; Inode, hard-links, & file size (. and , are for the decimal point,
depending on locale)
   ;; See comment for `directory-listing-before-filename-regexp' in `files.el'
or `files+.el'.
   '("\\(\\([0-9]+\\([.,][0-9]+\\)?\\)[BkKMGTPEZY]? \\)" 1 diredp-number)
 
   ;; Directory names
   (list "^..\\([0-9]* \\)*d"
         (list dired-move-to-filename-regexp nil nil)
         (list "\\(.+\\)" nil nil '(0 diredp-dir-priv t t)))
   '("^..\\([0-9]* \\)*.........\\(x\\)" 2 diredp-exec-priv) ;o x
   '("^..\\([0-9]* \\)*.........\\([lsStT]\\)" 2 diredp-other-priv) ; o misc
   '("^..\\([0-9]* \\)*........\\(w\\)" 2 diredp-write-priv) ; o w
   '("^..\\([0-9]* \\)*.......\\(r\\)" 2 diredp-read-priv) ; o r
   '("^..\\([0-9]* \\)*......\\(x\\)" 2 diredp-exec-priv) ; g x
   '("^..\\([0-9]* \\)*....[^0-9].\\([lsStT]\\)" 2 diredp-other-priv) ; g misc
   '("^..\\([0-9]* \\)*.....\\(w\\)" 2 diredp-write-priv) ; g w
   '("^..\\([0-9]* \\)*....\\(r\\)" 2 diredp-read-priv) ; g r
   '("^..\\([0-9]* \\)*...\\(x\\)" 2 diredp-exec-priv) ; u x
   '("^..\\([0-9]* \\)*...\\([lsStT]\\)" 2 diredp-other-priv) ; u misc
   '("^..\\([0-9]* \\)*..\\(w\\)" 2 diredp-write-priv) ; u w
   '("^..\\([0-9]* \\)*.\\(r\\)" 2 diredp-read-priv) ; u r
   '("^..\\([0-9]* \\)*.\\([-rwxlsStT]+\\)" 2 diredp-no-priv keep) ;-
   '("^..\\([0-9]* \\)*\\([bcsmpS]\\)[-rwxlsStT]" 2 diredp-rare-priv) ; (rare)
   '("^..\\([0-9]* \\)*\\(l\\)[-rwxlsStT]" 2 diredp-link-priv) ; l
   (list (concat "^\\([^\n " (char-to-string dired-del-marker) "].*$\\)")
         1 diredp-flag-mark-line t)     ; Flag/mark lines
   (list (concat "^\\([" (char-to-string dired-del-marker) "]\\)") ; Deletion
flags (D)
         '(1 diredp-deletion t)
         '(".+" (dired-move-to-filename) nil (0 diredp-deletion-file-name t)))
   (list (concat "^\\([^\n " (char-to-string dired-del-marker) "]\\)") ; Flags,
marks (except D)
         1 diredp-flag-mark t)
   ) "2nd level of Dired highlighting.  See `font-lock-maximum-decoration'.")
 
The local value of `font-lock-defaults' is correctly set to this value, as
it is in the earlier pretest #1 and in all prior Emacs releases:
 
((dired-font-lock-keywords diredp-font-lock-keywords-1)
 t nil nil beginning-of-line)
 
But none of the specified font-locking is manifested.  I see only the
default emacs -Q font-locking.  I don't know why, and am not sure how
to check this further.  
 
I've diff'd dired.el, dired-x.el, dired-aux.el, and font-lock.el between
the first pretest #1 delivery (2011-10-24) and the second one (2011-10-31),
but I don't recognize any change that would affect this.  Dunno what other
code changes were made between these two pretest deliveries, but something
broke font-locking badly in this context.
 

In GNU Emacs 24.0.91.1 (i386-mingw-nt5.1.2600) of 2011-10-30 on MARVIN
 Windowing system distributor `Microsoft Corp.', version 5.1.2600
 configured using `configure --with-gcc (4.6) --no-opt --cflags
 -I"D:/devel/emacs/libs/libXpm-3.5.8/include"
 -I"D:/devel/emacs/libs/libXpm-3.5.8/src"
 -I"D:/devel/emacs/libs/libpng-dev_1.4.3-1/include"
 -I"D:/devel/emacs/libs/zlib-dev_1.2.5-2/include"
 -I"D:/devel/emacs/libs/giflib-4.1.4-1/include"
 -I"D:/devel/emacs/libs/jpeg-6b-4/include"
 -I"D:/devel/emacs/libs/tiff-3.8.2-1/include"
 -I"D:/devel/emacs/libs/gnutls-2.10.1/include" --ldflags
 -L"D:/devel/emacs/libs/gnutls-2.10.1/lib"'
 

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

Note: If I do M-: (font-lock-refresh-defaults) in Dired then I get the
highlighting I should (already) have gotten according to `font-lock-defaults'.

Why is this extra call suddenly necessary?  What new Emacs 24 "feature" does
this map to?  Where is the Dev log that corresponds to this change? (I see no
change in font-lock.el, for instance.)

If this user-visible change is intended, then please (a) update the doc to
explain it, and (b) mention the change in NEWS.

(BTW, the name `font-lock-refresh-defaults' seems to be unfortunate.  Apparently
this function does _not_ refresh the font-lock defaults.  Instead, it refreshes
`font-lock-keywords' (and perhaps other things?) based on the current value of
`font-lock-defaults'.  The doc string even says the same thing: "... after
recomputing from defaults".  Recomputing font locking _from defaults_ is very
different from refreshing the defaults.  The value of `font-lock-defaults' is
not "refreshed" or otherwise changed in any way.)

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

> If this user-visible change is intended, then please (a) 
> update the doc to explain it, and (b) mention the change in NEWS.

Whatever the motivation behind this change (assuming it was intentional), the
effect is that font-locking now gets done _twice_ when someone modifies
`font-lock-defaults', because of the need to call `font-lock-refresh-defaults'.
In my case, this happens each time a Dired buffer is visited.  (Likewise buffer
menu and finder and...)

This is a waste of time.  It should be possible to simply update
`font-lock-defaults' (e.g. in a mode hook) and have font-locking be done only
once, using the updated defaults.  Why impose double font-locking?  Why should
any initial font-locking with the wrong defaults followed by "refreshing" to the
right ones be needed?  This apparently was not needed in the past.

It was such a simple idiom:

(set (make-local-variable 'font-lock-defaults)
     '(nil t nil nil nil
       (font-lock-fontify-region-function .
        browse-kill-ring-fontify-region)))

(setq font-lock-defaults  '(buffer-menu-font-lock-keywords t))

(setq font-lock-defaults '(finder-font-lock-keywords nil nil
                           (("+-*/.<>=!?$%_&~^:@" . "w")) nil))

(set (make-local-variable 'font-lock-defaults)
     '(compilation-mode-font-lock-keywords t))

...

On n'arrete pas le progres...

bug#9919: 24.0.91; font-lock broken in dired

[Christoph Scholtes]

"Drew Adams" <drew.adams@oracle.com> writes:

> This regression started with the second delivery of pretest #1,
> 2011-10-31.  There is no such problem with the first delivery,
> 2011-10-24.
>  
> In my setup I use dired+.el, which is here:
> http://www.emacswiki.org/emacs/download/dired%2b.el.

> [...]

> But none of the specified font-locking is manifested.  I see only the
> default emacs -Q font-locking.  I don't know why, and am not sure how
> to check this further.  

Drew,

I cannot reproduce this with the 24.0.91 Windows Pretest binary or my
current trunk build.

I downloaded dired+.el and put it in the Emacs path. With emacs -Q, I do
M-x load-library and load dired+.el. The dired buffer shows your custom
font-locking.

I then tried (require 'dired+) in my init.el and it also showed the
font-locking correctly when invoking dired.

Could anything have changed in your setup?

Christoph

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

> I cannot reproduce this with the 24.0.91 Windows Pretest binary or my
> current trunk build.
> I downloaded dired+.el and put it in the Emacs path. With 
> emacs -Q, I do M-x load-library and load dired+.el. The dired
> buffer shows your custom font-locking.
> I then tried (require 'dired+) in my init.el and it also showed the
> font-locking correctly when invoking dired.
> Could anything have changed in your setup?

No nothing changed in my setup.  But I added the workaround to dired+.el, as I
mentioned, so you will not see the bug with the latest dired+.el.  See the
2011/10/21 change-log entry.

To see the bug, just remove the call to `font-lock-refresh-defaults'.  There is
only one such call, preceded by this comment:
;; Emacs 24+: Need to refresh `font-lock-keywords' from `font-lock-defaults'.

(BTW, this bug was also reported to me by dired+.el users. It has nothing to do
with my setup.)

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

> > I cannot reproduce this with the 24.0.91 Windows Pretest 
> > binary or my current trunk build.
> > I downloaded dired+.el and put it in the Emacs path. With 
> > emacs -Q, I do M-x load-library and load dired+.el. The dired
> > buffer shows your custom font-locking.
> > I then tried (require 'dired+) in my init.el and it also showed the
> > font-locking correctly when invoking dired.
> > Could anything have changed in your setup?
> 
> No nothing changed in my setup.  But I added the workaround 
> to dired+.el, as I mentioned, so you will not see the bug with the latest 
> dired+.el.  See the 2011/10/21 change-log entry.
> 
> To see the bug, just remove the call to `font-lock-refresh-defaults'.
> There is only one such call, preceded by this comment:
> ;; Emacs 24+: Need to refresh `font-lock-keywords' from
> `font-lock-defaults'.
> 
> (BTW, this bug was also reported to me by dired+.el users. It 
> has nothing to do with my setup.)

ping.

I am still getting reports about this wrt dired+.el from users on GNU/Linux.  So
perhaps my workaround is not sufficient there - dunno.  In any case, this is a
regression and a performance problem.  It should not be necessary to explicitly
refontify (fontify twice).

bug#9919: 24.0.91; font-lock broken in dired

[Stefan Monnier]

> I am still getting reports about this wrt dired+.el from users on
> GNU/Linux.  So perhaps my workaround is not sufficient there - dunno.

Whenever you change font-lock-defaults, you need to cause
font-lock-keywords (and the text already font-locked) to be refreshed,
unless you know for sure that font-lock-mode has not yet been started.

> In any case, this is a regression and a performance problem.

I don't see any evidence of a performance problem.


        Stefan

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

> > I am still getting reports about this wrt dired+.el from users on
> > GNU/Linux.  So perhaps my workaround is not sufficient there - dunno.
> 
> Whenever you change font-lock-defaults, you need to cause
> font-lock-keywords (and the text already font-locked) to be refreshed,
> unless you know for sure that font-lock-mode has not yet been started.

Your "whenever" obviously does not apply to Emacs 20-22, since
`font-lock-refresh-defaults' does not even exist in those releases.

And at least for my code, "whenever" is true only for Emacs 24, not Emacs 23
(which also has `font-lock-refresh-defaults').  And the problem did not exist in
Emacs 24 either, until the build where I reported the bug.

I have this code, which has always worked and still works with Emacs 20 through
23.3:

(add-hook
  'dired-mode-hook
  (lambda ()
    (set (make-local-variable 'font-lock-defaults)
         (cons '(dired-font-lock-keywords diredp-font-lock-keywords-1)
               (cdr font-lock-defaults)))

    ;; Emacs 24+: Need to refresh `font-lock-keywords' from
`font-lock-defaults'.
    (when (fboundp 'font-lock-refresh-defaults) (font-lock-refresh-defaults))))

The last bit, which invokes `font-lock-refresh-defaults', I had to add for Emacs
24.

Similarly, for buff-menu+.el, I need to do likewise on `buffer-menu-mode-hook'.
Again, this workaround is needed only for Emacs 24.

> > In any case, this is a regression and a performance problem.
> 
> I don't see any evidence of a performance problem.

This means, in effect, that fontification must now be done twice, or else it is
incorrect.  That's the performance hit I meant: font-locking twice.  What has
always given correct fontification before with a single fontification (and still
does, in the published Emacs releases) now does not, I must re-fontify
completely.

Am I missing something here?  Is there a way to get what's needed without
fontifying twice?  That's expensive.  What's the cause of this
change/regression?  Thx.

bug#9919: 24.0.91; font-lock broken in dired

[Stefan Monnier]

>> > I am still getting reports about this wrt dired+.el from users on
>> > GNU/Linux.  So perhaps my workaround is not sufficient there - dunno.
>> Whenever you change font-lock-defaults, you need to cause
>> font-lock-keywords (and the text already font-locked) to be refreshed,
>> unless you know for sure that font-lock-mode has not yet been started.
> Your "whenever" obviously does not apply to Emacs 20-22, since
> `font-lock-refresh-defaults' does not even exist in those releases.

It does: notice I don't mention font-lock-refresh-defaults which merely
makes it easy (it was introduced just to make it easier to solve this
problem).

> And at least for my code, "whenever" is true only for Emacs 24, not
> Emacs 23 (which also has `font-lock-refresh-defaults').  And the
> problem did not exist in Emacs 24 either, until the build where
> I reported the bug.

No, your code was wrong and always has been: you change
only font-lock-defaults, whereas you do not know for a fact that
font-lock-mode has not yet been enabled.
In earlier Emacsen, in normal circumstances, you just got lucky.

> I have this code, which has always worked and still works with Emacs
> 20 through 23.3:

Code that works in practice doesn't mean correct code.

> (add-hook
>   'dired-mode-hook
>   (lambda ()
>     (set (make-local-variable 'font-lock-defaults)
>          (cons '(dired-font-lock-keywords diredp-font-lock-keywords-1)
>                (cdr font-lock-defaults)))

Great.  Now after running the above code, the user adds

  (add-hook 'dired-mode-hook (lambda () (font-lock-mode 1)))

and suddenly the bug shows up even in Emacs-23.

>> I don't see any evidence of a performance problem.
> This means, in effect, that fontification must now be done twice, or
> else it is incorrect.  That's the performance hit I meant:
> font-locking twice.

Evidence of a performance problem requires measures, times, things
like that.  You're talking about some hypothetical problem based on your
understanding of how things work.  Things don't always work the way we
think they do.  We have plenty of hypothetical performance problems in
Emacs that I'm not going to worry about them.

> What's the cause of this change/regression?

Nothing very deep.  The moment when font-lock-mode gets enabled
has changed.  I'm not convinced the new time is really much better, but
I'm not convinced the old time was better either.  Basically it used to
be enabled after running the mode hook, which made it
difficult to disable font-lock-mode via a mode-hook check, wherea now it
gets enabled before, so you can just do

  (add-hook 'dired-mode-hook (lambda () (if foo (font-lock-mode -1))))


-- Stefan

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

> > (add-hook
> >   'dired-mode-hook
> >   (lambda ()
> >     (set (make-local-variable 'font-lock-defaults)
> >          (cons '(dired-font-lock-keywords 
> >                   diredp-font-lock-keywords-1)
> >                (cdr font-lock-defaults)))
> 
> Now after running the above code, the user adds
>   (add-hook 'dired-mode-hook (lambda () (font-lock-mode 1)))
> and suddenly the bug shows up even in Emacs-23.

OK, so Emacs broke it in 23 instead of 24. ;-)  That does not happen in Emacs
20-22 AFAICT.  What's the gotcha for Emacs 20-22?  What does a user need to do
in Emacs 20-22 to show that this code won't DTRT?

Let me put it this way: I've never seen the problem in Emacs 20-22, and I've
never received a report from anyone mentioning such a problem with that code.

Did you receive any Emacs 20-22 bug reports, for example, from users who, like
me, did not understand that such problematic behavior was inherent?

Not that such reports would have corresponded to real bugs, from what you say,
but presumably there would have been at least someone who was bitten by what you
say has always been an inherent gotcha/problem.

I'm just curious why this is manifested in 24 without doing anything, and in 23
if you do as you note above, but not at all (AFAICT) in 20-22.

> > What's the cause of this change/regression?
> 
> Nothing very deep.  The moment when font-lock-mode gets enabled
> has changed.  I'm not convinced the new time is really much 
> better, but I'm not convinced the old time was better either.

Was that change made in Emacs 23 or 24?  See my question above - just trying to
understand the differences between (a) 20-22, (b) 23, and (c) 24.

> Basically it used to be enabled after running the mode hook,
> which made it difficult to disable font-lock-mode via a
> mode-hook check, wherea now it gets enabled before, so you can just do
> 
>   (add-hook 'dired-mode-hook (lambda () (if foo (font-lock-mode -1))))

I see.  Thanks for the explanation.  Makes sense.

bug#9919: 24.0.91; font-lock broken in dired

[Stefan Monnier]

> OK, so Emacs broke it in 23 instead of 24. ;-)  That does not happen
> in Emacs 20-22 AFAICT.  What's the gotcha for Emacs 20-22?  What does
> a user need to do in Emacs 20-22 to show that this code won't DTRT?

AFAIK the example will break your code in any version of Emacs
that supports font-lock.

>> Nothing very deep.  The moment when font-lock-mode gets enabled
>> has changed.  I'm not convinced the new time is really much 
>> better, but I'm not convinced the old time was better either.
> Was that change made in Emacs 23 or 24?

Emacs-24.
The change is to global-font-lock-mode, BTW.


        Stefan

bug#9919: 24.0.91; font-lock broken in dired

[Drew Adams]

> > That does not happen in Emacs 20-22 AFAICT.
> > What's the gotcha for Emacs 20-22?  What does a user need to
> > do in Emacs 20-22 to show that this code won't DTRT?
> 
> AFAIK the example will break your code in any version of Emacs
> that supports font-lock.

Nope, not the recipe you gave, at least.  Try it.
That's why I asked (still asking).

> >> Nothing very deep.  The moment when font-lock-mode gets enabled
> >> has changed.  I'm not convinced the new time is really much 
> >> better, but I'm not convinced the old time was better either.
> > Was that change made in Emacs 23 or 24?
> 
> Emacs-24.
> The change is to global-font-lock-mode, BTW.

Hm. I knew about that change, but didn't understand the consequences.

You can close this as not-a-bug, if you like.  Thx. 

bug#9919: 24.0.91; font-lock broken in dired

[Stefan Monnier]

tags 9919 notabug
thanks

>> > That does not happen in Emacs 20-22 AFAICT.
>> > What's the gotcha for Emacs 20-22?  What does a user need to
>> > do in Emacs 20-22 to show that this code won't DTRT?
>> AFAIK the example will break your code in any version of Emacs
>> that supports font-lock.
> Nope, not the recipe you gave, at least.  Try it.
> That's why I asked (still asking).

Don't know, then, there must be something else at play.


        Stefan