Bug in Elisp font-locking (was: Concern around use of eval)

Bug in Elisp font-locking (was: Concern around use of eval)

[Tassilo Horn]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

>>     (defun lh/gen-predicate (label)
>>       (defalias (intern (concat "lh/" label "-p"))
>                   ^^^^^^
> In this case intern should not be highlighted, isn't it ?.

Indeed, it shouldn't be.  But the font-lock entry for definitions which
applies the function name face here matches next to `defun', `defmacro',
or `defalias' also `cl-defstruct', and that may have the two forms

  (defstruct struct-name ...)
  (defstruct (struct-name OPTIONS) ...)

and therefore, the regexp simply skips the paren.  The following patch
should fix that.

Cc-ing emacs-devel to ask if that's ok to commit.  Is it?  Or too much
hassle for just `defalias'?  (I think that's the only definition form
which is implemented as a function where the name may be provided by a
funcall.)

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 6b30773..614fbc6 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -320,14 +320,18 @@
     `( ;; Definitions.
       (,(concat "(" el-defs-re "\\_>"
                 ;; Any whitespace and defined object.
-                "[ \t'\(]*"
-                "\\(\\(?:\\sw\\|\\s_\\)+\\)?")
+                "[ \t']*"
+               ;; With defstruct, the name may follow a paren,
+               ;; e.g. (defstruct (foo-struct opts)...).
+                "\\(([ \t']*\\)?\\(\\(?:\\sw\\|\\s_\\)+\\)?")
        (1 font-lock-keyword-face)
-       (2 (let ((type (get (intern-soft (match-string 1)) 'lisp-define-type)))
-            (cond ((eq type 'var) font-lock-variable-name-face)
-                  ((eq type 'type) font-lock-type-face)
-                  (t font-lock-function-name-face)))
-          nil t))
+       (3 (let ((type (get (intern-soft (match-string 1)) 'lisp-define-type)))
+           (cond ((eq type 'var) font-lock-variable-name-face)
+                 ((eq type 'type) font-lock-type-face)
+                 ;; If match-string 2 is non-nil, we encountered a
+                 ;; form like (defalias (intern (concat s "-p"))).
+                 ((not (match-string 2)) font-lock-function-name-face)))
+         nil t))
       ;; Emacs Lisp autoload cookies.  Supports the slightly different
       ;; forms used by mh-e, calendar, etc.
       ("^;;;###\\([-a-z]*autoload\\)" 1 font-lock-warning-face prepend))
--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo

Re: Bug in Elisp font-locking

[Stefan Monnier]

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>> (defun lh/gen-predicate (label)
>>> (defalias (intern (concat "lh/" label "-p"))
>> ^^^^^^
>> In this case intern should not be highlighted, isn't it ?.

Indeed.

>   (defstruct struct-name ...)
>   (defstruct (struct-name OPTIONS) ...)

Also for defun in Common-Lisp, there's (defun (setf foo) ...) where the
"setf" probably shouldn't be highlighted but the "foo" should.

> Cc-ing emacs-devel to ask if that's ok to commit.

Could explain what the patch does?


        Stefan

RE: Bug in Elisp font-locking

[Drew Adams]

How about dropping emacs-devel or help-gnu-emacs as recipient?
There should be no need to spread this over multiple lists, at
least at this point.  Please just move it to whichever you think
is most appropriate.

And if this is indeed about a bug, as the subject line says,
how about dropping them both and moving this to the bug tracker?

Re: Bug in Elisp font-locking

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> (defun lh/gen-predicate (label)
>>>> (defalias (intern (concat "lh/" label "-p"))
>>>             ^^^^^^
>>> In this case intern should not be highlighted, isn't it ?.
>
> Indeed.
>
>>   (defstruct struct-name ...)
>>   (defstruct (struct-name OPTIONS) ...)
>
> Also for defun in Common-Lisp, there's (defun (setf foo) ...) where
> the "setf" probably shouldn't be highlighted but the "foo" should.

The `setf' should be highlighted as a macro but not as function name as
it is right now.

>> Cc-ing emacs-devel to ask if that's ok to commit.
>
> Could explain what the patch does?

Instead of skipping over whitespace and opening parens between the
definition macro/function and the defined thing's name, it skips only
over whitespace but captures an optional opening paren.  If there's one,
then we don't font-lock with `font-lock-function-name-face'.

The valid cases where the definition macro/function defines a type like
in `cl-defstruct' still highlights the symbol after the paren (but in
`font-lock-type-face').

Or in simpler words: if the name appears after an opening paren, that
cannot be a function name so don't font-lock as such.

Bye,
Tassilo

Re: Bug in Elisp font-locking

[Stefan Monnier]

>> Could explain what the patch does?
[...]
> Or in simpler words: if the name appears after an opening paren, that
> cannot be a function name so don't font-lock as such.

OK, that sounds right for *Lisp,


        Stefan

Re: Bug in Elisp font-locking

[Tassilo Horn]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> Could explain what the patch does?
> [...]
>> Or in simpler words: if the name appears after an opening paren, that
>> cannot be a function name so don't font-lock as such.
>
> OK, that sounds right for *Lisp,

I've committed the patch for elisp, and just now also for CL after
scratching my head why its regex explicitly handled "(setf <symbol>" as
function name.  Aha, that's a setf-expander.  Learned something
again. :-)

Bye,
Tassilo

Re: Bug in Elisp font-locking

[Johan Bockgård]

Tassilo Horn <tsdh@gnu.org> writes:

> I've committed the patch for elisp, and just now also for CL after
> scratching my head why its regex explicitly handled "(setf <symbol>" as
> function name.

It *is* a function name, and you can use it like this:  #'(setf foo)

> Aha, that's a setf-expander.

Actually a "setf function", not a "setf expander".

Re: Bug in Elisp font-locking

[Stefan Monnier]

> scratching my head why its regex explicitly handled "(setf <symbol>" as
> function name.  Aha, that's a setf-expander.  Learned something
> again. :-)

It's supported for cl-defmethod, by the way.


        Stefan

Re: Bug in Elisp font-locking

[Tassilo Horn]

Johan Bockgård <bojohan@gnu.org> writes:

Hi Johan,

>> I've committed the patch for elisp, and just now also for CL after
>> scratching my head why its regex explicitly handled "(setf <symbol>" as
>> function name.
>
> It *is* a function name, and you can use it like this: #'(setf foo)

Interesting.

>> Aha, that's a setf-expander.
>
> Actually a "setf function", not a "setf expander".

Ah, yes.  Searching the net, I found out these setf functions are an
alternative to setf expanders for simpler cases.  Neat.  But we don't
have them in Elisp, not even in cl-defun, right?

Bye,
Tassilo

Re: Bug in Elisp font-locking

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> scratching my head why its regex explicitly handled "(setf <symbol>"
>> as function name.  Aha, that's a setf-expander.  Learned something
>> again. :-)
>
> It's supported for cl-defmethod, by the way.

Ah, there we have our counter-example.  Oh well, `cl-defmethod' isn't
listed as a defining keyword anyhow.  eieio's `defmethod' OTOH is, but
that doesn't seem to support (setf name), at least its docs don't
mention it.  So I guess the cl-version should be added since the eieio
version is marked as obsolete.

Anyhow, I've now added cl-defmethod as cl-lib definier and added the
(setf symbol) stuff also for elisp.

Bye,
Tassilo