bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Visuwesh]

In a skeleton template, it is not possible to pass `str' as an argument
to some functions.  To reproduce:

    1. emacs -Q

    2. Evaluate the following skeletons:

    (define-skeleton test-skeleton ""
      (file-relative-name (read-file-name "P: "))
      (shell-quote-argument str))

    (define-skeleton test-skeleton-2 ""
      (file-relative-name (read-file-name "P: "))
      (identity str))

    3. M-x test-skeleton RET RET -- witness it signal an error

    4. M-x test-skeleton-2 RET RET -- witness it insert "./"

I was able to reduce the problem down to the follwing sexp by edebbuging
skeleton-internal-1 (specifically the last `t' case in the cond signals
the error):

    (dlet ((str `(setq str "test"))
           (e `(shell-quote-argument str)))
      (eval e))

In GNU Emacs 29.0.91 (build 11, x86_64-pc-linux-gnu, X toolkit, Xaw
 scroll bars) of 2023-07-12 built on astatine
Repository revision: 17c7915ab947ebeec6ea5ad3eb4cad1f24d5d4fc
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Debian GNU/Linux trixie/sid

Configured using:
 'configure --with-sound=alsa --with-x-toolkit=lucid --with-json
 --without-xaw3d --without-gconf --without-libsystemd --without-cairo'

Configured features:
ACL DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XFT
XIM XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LC_MONETARY: ta_IN.UTF-8
  value of $LC_NUMERIC: ta_IN.UTF-8
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort help-mode emacsbug mail-extr message sendmail mailcap
yank-media puny dired dired-loaddefs rfc822 mml mml-sec password-cache
epa derived epg rfc6068 epg-config gnus-util text-property-search
time-date subr-x mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils
gmm-utils mailheader cl-loaddefs cl-lib skeleton rmc iso-transl tooltip
cconv eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow
isearch easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 41879 11878)
 (symbols 48 5521 1)
 (strings 32 14721 2050)
 (string-bytes 1 393181)
 (vectors 16 10382)
 (vector-slots 8 158438 14259)
 (floats 8 44 14)
 (intervals 56 314 11)
 (buffers 976 13))

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> In a skeleton template, it is not possible to pass `str' as an argument
> to some functions.

It is, but only after it appeared "naked" in the skeleton.

`skeleton-insert` says:

	str	first time: read a string according to INTERACTOR
		then: insert previously read string once more

which indeed doesn't explicitly tell you tht you're doing something
wrong, but it does imply that `str` doesn't just hold a string, or at
least not initially.

>     (define-skeleton test-skeleton ""
>       (file-relative-name (read-file-name "P: "))
>       (shell-quote-argument str))

It should work if you can change your skeleton to something like:

    (define-skeleton test-skeleton ""
      (file-relative-name (read-file-name "P: "))
      str
      (shell-quote-argument str))

where that first use of `str` will convert it from something that "read
a string according to INTERACTOR" to the actual string.


        Stefan

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Visuwesh]

[ Sorry about mangling the bug address, and thanks for fixing it, and
  replying.  ]

[திங்கள் செப்டம்பர் 11, 2023] Stefan Monnier wrote:

>> In a skeleton template, it is not possible to pass `str' as an argument
>> to some functions.
>
> It is, but only after it appeared "naked" in the skeleton.
>
> `skeleton-insert` says:
>
> 	str	first time: read a string according to INTERACTOR
> 		then: insert previously read string once more
>
> which indeed doesn't explicitly tell you tht you're doing something
> wrong, but it does imply that `str` doesn't just hold a string, or at
> least not initially.

Thanks, that explains the confusing part of the docstring.  However,
there's still a problem as...

>>     (define-skeleton test-skeleton ""
>>       (file-relative-name (read-file-name "P: "))
>>       (shell-quote-argument str))
>
> It should work if you can change your skeleton to something like:
>
>     (define-skeleton test-skeleton ""
>       (file-relative-name (read-file-name "P: "))
>       str
>       (shell-quote-argument str))
>
> where that first use of `str` will convert it from something that "read
> a string according to INTERACTOR" to the actual string.

... this inserts the directory twice.  :-(
AFAIU, using 'str should prevent the insertion of the value but that
signals the same error.  I tried (progn str (shell-quote-argument str))
instead but the same error again.

>
>         Stefan

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> It should work if you can change your skeleton to something like:
>>
>>     (define-skeleton test-skeleton ""
>>       (file-relative-name (read-file-name "P: "))
>>       str
>>       (shell-quote-argument str))
>>
>> where that first use of `str` will convert it from something that "read
>> a string according to INTERACTOR" to the actual string.
>
> ... this inserts the directory twice.  :-(

Of course, it changes the behavior of your skeleton.
I assumed that your *real* skeleton does something else anyway, so
I showed what kind of change might help.

To do what your above skeleton does, you simply can't use `str`.
You need something like:

    (define-skeleton test-skeleton ""
      nil
      (shell-quote-argument (file-relative-name (read-file-name "P: "))))

instead.

> AFAIU, using 'str should prevent the insertion of the value but that
> signals the same error.

Sorry, I don't know what you mean by that.

> I tried (progn str (shell-quote-argument str))
> instead but the same error again.

Of course: any use `str` within an actual expression (as opposed to
using it as a skeleton element) will work reliably only if that occurs
after a use of `str` as a skeleton element.


        Stefan

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Visuwesh]

[திங்கள் செப்டம்பர் 11, 2023] Stefan Monnier wrote:

>>> It should work if you can change your skeleton to something like:
>>>
>>>     (define-skeleton test-skeleton ""
>>>       (file-relative-name (read-file-name "P: "))
>>>       str
>>>       (shell-quote-argument str))
>>>
>>> where that first use of `str` will convert it from something that "read
>>> a string according to INTERACTOR" to the actual string.
>>
>> ... this inserts the directory twice.  :-(
>
> Of course, it changes the behavior of your skeleton.
> I assumed that your *real* skeleton does something else anyway, so
> I showed what kind of change might help.
>
> To do what your above skeleton does, you simply can't use `str`.
> You need something like:
>
>     (define-skeleton test-skeleton ""
>       nil
>       (shell-quote-argument (file-relative-name (read-file-name "P: "))))
>
> instead.

Yes, of course.  I end up having to do something like the let form below

    (defun vz/read-relative-filename (&optional prompt dir)
      (let ((file (read-file-name (or prompt "Filname: ") dir "")))
         (if (equal file "")
             ""
           (file-relative-name file default-directory))))

    (define-skeleton imagemagick-collage-images
      "Collage/montage multiple images using imagemagick."
      nil
      "montage "
      ((let ((file (vz/read-relative-filename "Image: ")))
         (if (equal file "")
             ""
           (shell-quote-argument file)))
       str " ")
      "-geometry 00 "
      "-tile " (skeleton-read "How many columns (horizontal)? ") "x"
      (skeleton-read "How many rows (vertical)? ")
      (shell-quote-argument (vz/read-relative-filename "Out: ")))

I was hoping I could avoid doing this.  I wanted to ask you why the eval
form used by skeleton errors but now I realise that str is a list when
it is eval'ed in the example I gave in the OP; I was thinking of eval as
some kind of macroexpansion instead.  I disturbed you all for nothing,
sorry.

I can do

    (define-skeleton test
      ""
      (read-file-name "p: ")
      (shell-quote-argument (eval str)))

instead and that works without the error.  I do wonder if the eval can
be avoided but this bug can be closed regardless.

>> AFAIU, using 'str should prevent the insertion of the value but that
>> signals the same error.
>
> Sorry, I don't know what you mean by that.

I meant this part in skeleton-insert's docstring

    Quoted Lisp expressions are evaluated for their side-effects.

>> I tried (progn str (shell-quote-argument str))
>> instead but the same error again.
>
> Of course: any use `str` within an actual expression (as opposed to
> using it as a skeleton element) will work reliably only if that occurs
> after a use of `str` as a skeleton element.
>
>
>         Stefan

Thanks for your patience and help.

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I can do
>
>     (define-skeleton test
>       ""
>       (read-file-name "p: ")
>       (shell-quote-argument (eval str)))

Eww... please don't!  This relies on an internal detail about how `str`
is implemented.

>>> AFAIU, using 'str should prevent the insertion of the value but that
>>> signals the same error.
>> Sorry, I don't know what you mean by that.
> I meant this part in skeleton-insert's docstring
>
>     Quoted Lisp expressions are evaluated for their side-effects.

Ah, I see.  Yes, using

    'str

should basically have no effect.

>>> I tried (progn str (shell-quote-argument str))
>>> instead but the same error again.
>> Of course: any use `str` within an actual expression (as opposed to
>> using it as a skeleton element) will work reliably only if that occurs
>> after a use of `str` as a skeleton element.
> Thanks for your patience and help.

We should try and improve the docs to clarify this confusing situation.
If you have suggestions for how/where we could make changes that would
have effectively discouraged you from trying to use `str` in that way,
I'd be happy to hear them.


        Stefan

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Visuwesh]

[திங்கள் செப்டம்பர் 11, 2023] Stefan Monnier wrote:

>>>> AFAIU, using 'str should prevent the insertion of the value but that
>>>> signals the same error.
>>> Sorry, I don't know what you mean by that.
>> I meant this part in skeleton-insert's docstring
>>
>>     Quoted Lisp expressions are evaluated for their side-effects.
>
> Ah, I see.  Yes, using
>
>     'str
>
> should basically have no effect.

Yes, but here it basically does nothing unfortunately.  I am not sure if

    'str

should have the side effect of setting that variable.

>>>> I tried (progn str (shell-quote-argument str))
>>>> instead but the same error again.
>>> Of course: any use `str` within an actual expression (as opposed to
>>> using it as a skeleton element) will work reliably only if that occurs
>>> after a use of `str` as a skeleton element.
>> Thanks for your patience and help.
>
> We should try and improve the docs to clarify this confusing situation.
> If you have suggestions for how/where we could make changes that would
> have effectively discouraged you from trying to use `str` in that way,
> I'd be happy to hear them.

Unfortunately, I don't really remember what motivated me to end up with
such a skeleton.  I usually have the problems I face written as a
comment but this time I didn't.  Looking at the skeletons I have, ISTM
that I want the skeleton to "end" whenever I give the empty string as
the value for `str'.  In fact, I have a macro defined

    (defmacro vz/snippet-when (form &rest body)
      "Evaluate BODY if FORM returns non-nil or non-empty string."
      (declare (indent 1) (debug (form body)))
      `(let ((str ,form))
         (if (and str (equal str ""))
             ""
           ,@body)))

and have skeleton bodies such as

    (define-skeleton imagemagick-collage-images
      "Collage/montage multiple images using imagemagick."
      nil
      "montage "
      ((vz/snippet-when (vz/snippet-read-relative-filename "Image: ")
         (shell-quote-argument str))
       str " ")
      "-geometry +0+0 "
      "-tile " (skeleton-read "How many columns (horizontal)? ") "x"
      (skeleton-read "How many rows (vertical)? ")
      (shell-quote-argument (vz/snippet-read-relative-filename "Out: ")))

using it.

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Yes, but here it basically does nothing unfortunately.  I am not sure if
>
>     'str
>
> should have the side effect of setting that variable.

The above skeleton expression means "eval the variable `str` and throw
away the result".  The only effect it can have is to signal an error if
the variable `str` is not bound.

>     (defmacro vz/snippet-when (form &rest body)
>       "Evaluate BODY if FORM returns non-nil or non-empty string."
>       (declare (indent 1) (debug (form body)))
>       `(let ((str ,form))
>          (if (and str (equal str ""))
>              ""
>            ,@body)))

FWIW, this style of macro is usually called "anaphoric macro" and the
convention is to use the identifier `it` rather than `str` in them.
It would help avoid a confusion between skeleton's `str` and your
macro's variable.

[ And, yes, arguably, `define-skeleton` could be considered as an
  anaphoric macro as well which should use `it` instead of `str`,
  bringing back the confusion :-)  ]


        Stefan

bug#64584: 29.0.91; skeleton: cannot pass `str' as argument to some functions

[Visuwesh]

[செவ்வாய் செப்டம்பர் 12, 2023] Stefan Monnier wrote:

>> Yes, but here it basically does nothing unfortunately.  I am not sure if
>>
>>     'str
>>
>> should have the side effect of setting that variable.
>
> The above skeleton expression means "eval the variable `str` and throw
> away the result".  The only effect it can have is to signal an error if
> the variable `str` is not bound.

Ah yes, of course.  I am still thinking in vague terms of macroexpansion
which is leading to all the misunderstandings.

>>     (defmacro vz/snippet-when (form &rest body)
>>       "Evaluate BODY if FORM returns non-nil or non-empty string."
>>       (declare (indent 1) (debug (form body)))
>>       `(let ((str ,form))
>>          (if (and str (equal str ""))
>>              ""
>>            ,@body)))
>
> FWIW, this style of macro is usually called "anaphoric macro" and the
> convention is to use the identifier `it` rather than `str` in them.
> It would help avoid a confusion between skeleton's `str` and your
> macro's variable.

Oh yes, I called `it' first but changed it `str' "in anger".  So far, I
haven't been in the need of using both vz/snippet-when's `str' and
skeleton's `str' so I will let the sleeping dogs lie.

> [ And, yes, arguably, `define-skeleton` could be considered as an
>   anaphoric macro as well which should use `it` instead of `str`,
>   bringing back the confusion :-)  ]

I guess it wasn't chosen because there are other variables too (like
help, v1, v2, etc.).

I cannot think of any improvements to the documentation since the spawn
of this bug report is rooted in a lack of understanding of how skeleton
works.  There is nothing to fix here except my understanding so I'm
closing the bug report with this.  Thank you again for your time,
Stefan.