bug#60730: 29.0.60; Free variable with :buffer keyword in ert-with-temp-file

[J.P.]

Eli Zaretskii <eliz@gnu.org> writes:

>> "J.P." <jp@neverwas.me> writes:
>> 
>> If, as you say, an argument to `:coding' should only ever be quoted, e.g.,
>> 
>>   :coding 'raw-text
>> 
>> then `coding' will end up quoted as well, so something like this might
>> be enough:
>
> If you say so.  The `', stuff looks strange to me, but the backticks
> in Emacs Lisp have always been black magic.
>
> What we need to ensure that both
>
>   :coding 'raw-text
>
> and
>
>   :coding some-coding-variable
>
> do work as expected, including when coding-system-for-write's value is
> a non-nil symbol of a coding-system.

Right, whatever the solution, it should cover those bases. Although, if
`some-coding-variable' evaluates to nil, the change I proposed would not
fall back on `coding-system-for-write'. (But perhaps it should? [1])

Also, thinking about this in earnest (for once), I'm unsure why we need
to capture the value of `coding-system-for-write' at expansion time.
Wouldn't it be preferable to defer evaluation until the test actually
runs? IOW, when the `:coding' keyword is absent, shouldn't the final
form contain

  -> (let* ((coding-system-for-write coding-system-for-write) ...

or even

  -> (let* (...

(meaning nothing)? If this "deferred" approach makes sense, perhaps
something like this will suffice:

  diff --git a/lisp/emacs-lisp/ert-x.el b/lisp/emacs-lisp/ert-x.el
  index 98a017c8a8e..2605fc22ddf 100644
  --- a/lisp/emacs-lisp/ert-x.el
  +++ b/lisp/emacs-lisp/ert-x.el
  @@ -475,7 +475,7 @@ ert-with-temp-file
           (:directory (setq directory (pop body)))
           (:text (setq text (pop body)))
           (:buffer (setq buffer (pop body)))
  -        (:coding (setq coding (pop body)))
  +        (:coding (setq coding (list (pop body))))
           (_ (push keyw extra-keywords) (pop body))))
       (when extra-keywords
         (error "Invalid keywords: %s" (mapconcat #'symbol-name extra-keywords 
" ")))
  @@ -484,7 +484,7 @@ ert-with-temp-file
             (suffix (or suffix ert-temp-file-suffix
                         (ert--with-temp-file-generate-suffix
                          (or (macroexp-file-name) buffer-file-name)))))
  -      `(let* ((coding-system-for-write ,(or coding coding-system-for-write))
  +      `(let* (,@(and coding `((coding-system-for-write ,(car coding))))
                 (,temp-file (,(if directory 'file-name-as-directory 'identity)
                              (make-temp-file ,prefix ,directory ,suffix 
,text)))
                 (,name ,(if directory

Note that with this change, `coding-system-for-write' would only be
bound when the user supplies a `:coding' argument, even if that argument
is nil [2]. Anyway, if this "deferred" stuff is simply wrongheaded,
please forget I ever mentioned it. Thanks.


[1] If incorporating such "fallback" behavior into the "deferred"
    approach mentioned above is desirable, we could try something
    like

      diff --git a/lisp/emacs-lisp/ert-x.el b/lisp/emacs-lisp/ert-x.el
      index 98a017c8a8e..3439aacf1ab 100644
      --- a/lisp/emacs-lisp/ert-x.el
      +++ b/lisp/emacs-lisp/ert-x.el
      @@ -484,7 +484,7 @@ ert-with-temp-file
                 (suffix (or suffix ert-temp-file-suffix
                             (ert--with-temp-file-generate-suffix
                              (or (macroexp-file-name) buffer-file-name)))))
      -      `(let* ((coding-system-for-write ,(or coding 
coding-system-for-write))
      +      `(let* ((coding-system-for-write (or ,coding 
coding-system-for-write))
                     (,temp-file (,(if directory 'file-name-as-directory 
'identity)
                                  (make-temp-file ,prefix ,directory ,suffix 
,text)))
                     (,name ,(if directory

[2] My main concern with the "fallback" route is that the user loses a
    rather convenient means of ignoring whatever value of
    `coding-system-for-write' exists in their testing environment. IOW,
    they cannot easily opt to favor the default selection procedure
    mentioned in the doc string (for `c-s-f-w'). As a user of this
    macro, I feel it might be handy to have the option of supplying a
    literal nil (or a variable evaluating to nil) to signal such intent.