Concern around use of eval

Concern around use of eval

[Les Harris]

I have a situation where I want to generate a bunch of nearly identical
predicate functions. Previously I had just manually defined these things
one by one but was never happy with such an obvious inefficiency (or
maintaining it). Tonight I decided I'd fix this and whacked them and
implemented a way of doing it where I can iterate over a list containing
labels and define these predicate functions using those labels:

,----
| (defvar lh/labels '("label1" "label2" "label3"))
| 
| (defmacro lh/gen-predicate (label)
|   `(defun ,(intern (concat "lh/" label "-p")) ()
|             (member ,label *lh/system-label-store*)))
| 
| (defun lh/define-predicates ()
|   (dolist (label lh/labels)
|     (eval `(lh/gen-predicate ,label))))
| (lh/define-predicates)
`----

Now this all works fine and I get my auto-generated predicates, so
success. My question/concern/niggle is around the use of (eval) in
lh/define-predicates. If I don't put eval in there then the defun the
macro evaluates into never gets evaluated itself. Stylistically, is
there a better way to do this or am I just being weird about (eval) and
should just get over it?

-- 
Do they only stand
By ignorance, is that their happy state,
The proof of their obedience and their faith?

Re: Concern around use of eval

[Tassilo Horn]

Les Harris <les@lesharris.com> writes:

Hi Les,

> ,----
> | (defvar lh/labels '("label1" "label2" "label3"))
> | 
> | (defmacro lh/gen-predicate (label)
> |   `(defun ,(intern (concat "lh/" label "-p")) ()
> |             (member ,label *lh/system-label-store*)))
> | 
> | (defun lh/define-predicates ()
> |   (dolist (label lh/labels)
> |     (eval `(lh/gen-predicate ,label))))
> | (lh/define-predicates)
> `----
>
> Now this all works fine and I get my auto-generated predicates, so
> success. My question/concern/niggle is around the use of (eval) in
> lh/define-predicates. If I don't put eval in there then the defun the
> macro evaluates into never gets evaluated itself. Stylistically, is
> there a better way to do this or am I just being weird about (eval) and
> should just get over it?

Well, in case the `lh/label' value is available at compile-time then
`lh/define-predicates' can also be a macro and you can go like this:

--8<---------------cut here---------------start------------->8---
(defvar lh/labels '("label1" "label2" "label3"))

(defmacro lh/gen-predicate (label)
  `(defun ,(intern (concat "lh/" label "-p")) ()
     (member ,label *lh/system-label-store*)))

(defmacro lh/define-predicates ()
  `(progn
     ,@(mapcar (lambda (label)
		 `(lh/gen-predicate ,label))
	       lh/labels)))

(lh/define-predicates)
--8<---------------cut here---------------end--------------->8---

But if you know your labels only at runtime, then I think there's no way
around using `eval'.

Bye,
Tassilo

Re: Concern around use of eval

[Joost Kremers]

Tassilo Horn wrote:
> Well, in case the `lh/label' value is available at compile-time then
> `lh/define-predicates' can also be a macro and you can go like this:
>
> --8<---------------cut here---------------start------------->8---
> (defvar lh/labels '("label1" "label2" "label3"))
>
> (defmacro lh/gen-predicate (label)
>   `(defun ,(intern (concat "lh/" label "-p")) ()
>      (member ,label *lh/system-label-store*)))
>
> (defmacro lh/define-predicates ()
>   `(progn
>      ,@(mapcar (lambda (label)
> 		 `(lh/gen-predicate ,label))
> 	       lh/labels)))
>
> (lh/define-predicates)
> --8<---------------cut here---------------end--------------->8---

Why not do it at the top level directly? E.g.:

,----
| (defvar lh/labels '("label1" "label2" "label3"))
| 
| (defmacro lh/gen-predicate (label)
|   `(defun ,(intern (concat "lh/" label "-p")) ()
|      (member ,label *lh/system-label-store*)))
| 
| (mapc (lambda (label)
|         (lh/gen-predicate label))
|       lh/labels)
`----


-- 
Joost Kremers                                   joostkremers@fastmail.fm
Selbst in die Unterwelt dringt durch Spalten Licht
EN:SiS(9)

Re: Concern around use of eval

[Tassilo Horn]

Joost Kremers <joost.m.kremers@gmail.com> writes:

> Why not do it at the top level directly? E.g.:
>
> ,----
> | (defvar lh/labels '("label1" "label2" "label3"))
> | 
> | (defmacro lh/gen-predicate (label)
> |   `(defun ,(intern (concat "lh/" label "-p")) ()
> |      (member ,label *lh/system-label-store*)))
> | 
> | (mapc (lambda (label)
> |         (lh/gen-predicate label))
> |       lh/labels)
> `----

That won't work because.  When reading the mapc form, emacs sees that
`lh/gen-predicate' is a macro and tries to expand it with the symbol
label as argument.  The macroexpansion takes place before the actual
execution of the code.

Bye,
Tassilo

Re: Concern around use of eval

[Stefan Monnier]

> | (defvar lh/labels '("label1" "label2" "label3"))
> | 
> | (defmacro lh/gen-predicate (label)
> |   `(defun ,(intern (concat "lh/" label "-p")) ()
> |             (member ,label *lh/system-label-store*)))
> | 
> | (defun lh/define-predicates ()
> |   (dolist (label lh/labels)
> |     (eval `(lh/gen-predicate ,label))))
> | (lh/define-predicates)

    ; -*- lexical-binding:t -*-

    (defvar lh/labels '("label1" "label2" "label3"))
    
    (defun lh/gen-predicate (label)
      (defalias (intern (concat "lh/" label "-p"))
        (lambda () (member label *lh/system-label-store*))))
    
    (defun lh/define-predicates ()
      (mapc #'lh/gen-predicate lh/labels))

    (lh/define-predicates)

> Stylistically, is there a better way to do this or am I just being
> weird about (eval) and should just get over it?

You're not weird about `eval'.  It's kind of like self-modifying code:
there are cases where you *really* need it, but they're extremely rare
and every time you use it when it's not *really* indispensable,
kittens die.


        Stefan

Re: Concern around use of eval

[Thierry Volpiatto]

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 ?.

-- 
Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997 

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: Concern around use of eval

[Joost Kremers]

Tassilo Horn wrote:
> Joost Kremers <joost.m.kremers@gmail.com> writes:
>
>> Why not do it at the top level directly? E.g.:
>>
>> ,----
>> | (defvar lh/labels '("label1" "label2" "label3"))
>> | 
>> | (defmacro lh/gen-predicate (label)
>> |   `(defun ,(intern (concat "lh/" label "-p")) ()
>> |      (member ,label *lh/system-label-store*)))
>> | 
>> | (mapc (lambda (label)
>> |         (lh/gen-predicate label))
>> |       lh/labels)
>> `----
>
> That won't work because.  When reading the mapc form, emacs sees that
> `lh/gen-predicate' is a macro and tries to expand it with the symbol
> label as argument.  The macroexpansion takes place before the actual
> execution of the code.

Thanks, that explains a few things.


-- 
Joost Kremers                                   joostkremers@fastmail.fm
Selbst in die Unterwelt dringt durch Spalten Licht
EN:SiS(9)

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