Gnus using lexical-binding

Gnus using lexical-binding

[Stefan Monnier]

I'm working on converting all the lisp/gnus files to use
lexical-binding.  I have a first version of the code ready and pushed to
`scratch/lexical-gnus`.  It works for me, but please test it because my
use of Gnus only exercises a small fraction of its functionality
and packages.

And please, after testing it, tell me how it went, regardless if you
encountered problems or not.

I'll be rebasing that branch as I split it into a few separate commits
to clean up its history, so don't expect to be able to track it
with merges.


        Stefan

Re: Gnus using lexical-binding

[Lars Ingebrigtsen]

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

> I'm working on converting all the lisp/gnus files to use
> lexical-binding.  I have a first version of the code ready and pushed to
> `scratch/lexical-gnus`.  It works for me, but please test it because my
> use of Gnus only exercises a small fraction of its functionality
> and packages.

I gave it a brief spin now, and I didn't encounter any problems.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Gnus using lexical-binding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 29 Jan 2021 18:11:35 -0500
> 
> I'm working on converting all the lisp/gnus files to use
> lexical-binding.  I have a first version of the code ready and pushed to
> `scratch/lexical-gnus`.  It works for me, but please test it because my
> use of Gnus only exercises a small fraction of its functionality
> and packages.

Thanks.

Btw, I think we could benefit from a detailed guide for how to convert
a package to lexical-binding, enumerating the steps to take, tools and
Emacs commands to use, possible problems and how to solve them, etc.
Somewhere in admin/notes/, perhaps?

Re: Gnus using lexical-binding

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Btw, I think we could benefit from a detailed guide for how to convert
> a package to lexical-binding, enumerating the steps to take, tools and
> Emacs commands to use, possible problems and how to solve them, etc.
> Somewhere in admin/notes/, perhaps?

What about `(elisp) Converting to Lexical Binding'?

Should we perhaps just expand that?  I'm sure third-party package
developers could also benefit from any details we would like to add.

Re: Gnus using lexical-binding

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 30 Jan 2021 05:43:13 -0800
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Btw, I think we could benefit from a detailed guide for how to convert
> > a package to lexical-binding, enumerating the steps to take, tools and
> > Emacs commands to use, possible problems and how to solve them, etc.
> > Somewhere in admin/notes/, perhaps?
> 
> What about `(elisp) Converting to Lexical Binding'?

Is that all we have to say on that topic?

It also says "experimental" about some part of it, which to me spells
"use at your own peril".

> Should we perhaps just expand that?  I'm sure third-party package
> developers could also benefit from any details we would like to add.

I think any experience with this job should be beneficial.  Not sure
it all belongs to the manual, though, which is why I suggested to put
it in admin/notes.

Thanks.

Re: Gnus using lexical-binding

[Eli Zaretskii]

> Date: Sat, 30 Jan 2021 15:52:15 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> > What about `(elisp) Converting to Lexical Binding'?
> 
> Is that all we have to say on that topic?

And btw, why is this only on master?  Chances are, when Emacs 28 will
be out, most of the conversion will have happened already.  So people
will need this information *now*, and we should backport this to the
emacs-27 branch, I think.

Re: Gnus using lexical-binding

[Basil L. Contovounesios]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

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

> I'm working on converting all the lisp/gnus files to use
> lexical-binding.  I have a first version of the code ready and pushed to
> `scratch/lexical-gnus`.  It works for me, but please test it because my
> use of Gnus only exercises a small fraction of its functionality
> and packages.

Thanks.

> And please, after testing it, tell me how it went, regardless if you
> encountered problems or not.

Bootstrapping emits the following warnings:

  In mail-add-attachment:
  mail/sendmail.el:1809:25: Warning: ‘mm-default-file-encoding’ is an obsolete
      function (as of future); use ‘mm-default-file-type’ instead.

  In mh-minibuffer-read-type:
  mh-e/mh-mime.el:1728:24: Warning: ‘mm-default-file-encoding’ is an obsolete
      function (as of future); use ‘mm-default-file-type’ instead.

and starting Gnus only gets me as far as the splash screen; see the
attached backtrace, which I'm guessing is related to the following
setting:

  (setq-default
   gnus-group-line-format
   (concat "%M"           ; Marked articles
           "%S"           ; Subscription
           "%p"           ; Marked for processing
           "%m"           ; New mail
           "%B"           ; Open summary buffer
           "%P"           ; Topic indentation
           "%5y? %3T!"    ; Unread and ticked articles
           " : "          ; Colon
           "%(%-40,40c%)" ; Collapsed group name
           "%9u&dgroup;"  ; Last read
           "\n"))

(The last %-sequence is just a custom timestamp format.)

Let me know if you want me to test anything else out.

-- 
Basil


[-- Attachment #2: backtrace.el --]
[-- Type: application/emacs-lisp, Size: 2154 bytes --]

Re: Gnus using lexical-binding

[Stefan Monnier]

>> Btw, I think we could benefit from a detailed guide for how to convert
>> a package to lexical-binding, enumerating the steps to take, tools and
>> Emacs commands to use, possible problems and how to solve them, etc.
>> Somewhere in admin/notes/, perhaps?
>
> What about `(elisp) Converting to Lexical Binding'?
>
> Should we perhaps just expand that?  I'm sure third-party package
> developers could also benefit from any details we would like to add.

In terms of tools and commands, this section indeed describes what
I use.

I'm not sure how much further we want to go with that.
The main other tool I use is my intuition and various heuristics:

For the "unused var" warning, I can usually guess based on the name
whether it should have a `defvar` or not: variables whose name has
a "package prefix" should almost always have a `defvar` while those who
have a short name should almost always use lexical scoping.
Adding more `defvar`s than needed is largely harmless during conversion
(it's arguably harmful in the long run, but that can be addresses
later), so the main risk is for those variables with a non-prefixed name
where we normally presume they can use lexical scoping but which may
nevertheless require dynamic scoping.  Furthermore, some of those cases
can occur without triggering any warning from the byte-compiler because
the var is not locally unused.

This can happen typically with code like

    (let ((foo X)
          (bar Y))
      ...
      (eval config-var)
      ...)

where `config-var` is defined to hold an ELisp form which has access to
variables `foo` and `bar`.  If you're the package author or are a heavy
user of the package there's a chance you know about it and can take it into
account when performing the conversion, but otherwise it can be tricky
to figure it out.

The only way I know is to search for uses of `eval`, `symbol-value`,
`intern`, and `set` (this list is sadly not exhaustive, e.g. some uses
may occur via `run-hooks`) to try and find code that relies on access to
variables via dynamic scoping and then examine each one to try and see
which variables it may refer to, which is sometimes documented in
comments or docstrings.

This last part is tedious and rarely necessary, so I do it lazily,
i.e. when something indicates that it's probably necessary (e.g. because
of a regression found during testing, or because of some odd patterns in
the "unused variables" warnings, e.g. when the same set of unused
variable names appear a few times, tho sometimes these patterns are due
to copy&paste as well).


        Stefan

Re: Gnus using lexical-binding

[Andy Moreton]

On Fri 29 Jan 2021, Stefan Monnier wrote:

> I'm working on converting all the lisp/gnus files to use
> lexical-binding.  I have a first version of the code ready and pushed to
> `scratch/lexical-gnus`.  It works for me, but please test it because my
> use of Gnus only exercises a small fraction of its functionality
> and packages.
>
> And please, after testing it, tell me how it went, regardless if you
> encountered problems or not.

Fails starting gnus for me. Setting `debug-on-error' gives backtrace:

Debugger entered--Lisp error: (void-variable gnus-tmp-level)
  (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr (assq ... gnus-tmp-marked))) (gnus-range-length (cdr (assq ... gnus-tmp-marked)))))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)
  (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr ...)) (gnus-range-length (cdr ...))))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live))
  (let (gnus-position) (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length ...) (gnus-range-length ...)))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)) (setq gnus-position (point)) (insert (format "%-55s" (let* ((val (eval (if ... ... gnus-tmp-qualified-group))) (need (- 55 (string-width val)))) (if (> need 0) (concat nil val (make-string need 32)) val)))) (insert-char 32 (max (- 75 (current-column)) 0)) (add-text-properties (point) (progn (insert gnus-tmp-news-server) (point)) (cons 'face (cons (list 'italic 'default) '(gnus-face t)))) (insert "\n") (if gnus-position (put-text-property gnus-position 
 (1+ gnus-position) 'gnus-position t)))
  eval((let (gnus-position) (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number ... ...))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)) (setq gnus-position (point)) (insert (format "%-55s" (let* ((val (eval ...)) (need (- 55 ...))) (if (> need 0) (concat nil val (make-string need 32)) val)))) (insert-char 32 (max (- 75 (current-column)) 0)) (add-text-properties (point) (progn (insert gnus-tmp-news-server) (point)) (cons 'face (cons (list 'italic 'default) '(gnus-face t)))) (insert "\n") (if gnus-position (put-text-property gnus-position (1+ gnus-position) 'gnus-position t))) t)
  gnus-group-insert-group-line("nndraft:drafts" 1 nil 0 (nndraft ""))
  gnus-topic-prepare-topic((("misc" visible nil nil)) 1 5 nil nil 1 nil)
  gnus-topic-prepare-topic((("Topics" visible nil nil) (("Cygwin" visible nil nil)) (("Emacs" visible nil nil)) (("Windows" visible nil nil)) (("Linux" visible nil nil)) (("IETF" visible nil nil)) (("Network" visible nil nil)) (("Developer" visible nil nil) (("Build Tools" visible nil nil)) (("GCC" visible nil nil)) (("LLVM" visible nil nil)) (("Libc" visible nil nil))) (("Languages" visible nil nil) (("C++" visible nil nil)) (("Rust" visible nil nil)) (("Python" visible nil nil)) (("Lua" visible nil nil)) (("Javascript" visible nil nil)) (("Scheme" visible nil nil)) (("Common Lisp" visible nil nil)) (("Haskell" visible nil nil))) (("Hardware" visible nil nil)) (("Blogs" visible nil nil)) (("misc" visible nil nil))) 0 5 nil nil 1 nil)
  gnus-group-prepare-topics(5 nil nil)
  gnus-group-list-groups(nil)
  #f(compiled-function () #<bytecode 0x991f1d1e36f151a>)()
  gnus-1(nil nil nil)
  gnus(nil)
  funcall-interactively(gnus nil)
  call-interactively(gnus record nil)
  command-execute(gnus record)
  execute-extended-command(nil "gnus" "gnus")
  funcall-interactively(execute-extended-command nil "gnus" "gnus")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

Re: Gnus using lexical-binding

[Stefan Monnier]

> Fails starting gnus for me. Setting `debug-on-error' gives backtrace:
>
> Debugger entered--Lisp error: (void-variable gnus-tmp-level)

Thanks.  EMACS_DYNVARS_FILE caught this one earlier today as well, indeed.


        Stefan

Re: Gnus using lexical-binding

[Stefan Monnier]

> Bootstrapping emits the following warnings:
>
>   In mail-add-attachment:
>   mail/sendmail.el:1809:25: Warning: ‘mm-default-file-encoding’ is an obsolete
>       function (as of future); use ‘mm-default-file-type’ instead.
>
>   In mh-minibuffer-read-type:
>   mh-e/mh-mime.el:1728:24: Warning: ‘mm-default-file-encoding’ is an obsolete
>       function (as of future); use ‘mm-default-file-type’ instead.

Good catch, thanks.

> and starting Gnus only gets me as far as the splash screen; see the
> attached backtrace, which I'm guessing is related to the following
[...]
> Debugger entered--Lisp error: (void-variable gnus-tmp-marked)
[...]
>   gnus-group-insert-group-line("dummy.group" 0 nil 0 nil)

EMACS_DYNVARS_FILE had also found this one (actually, this one and the
`gnus-tmp-level` in that same `gnus-group.el` file are the only two new
problems that it found, so it was no better at finding problems than
Basil and Andy combined).

I've updated the branch accordingly.


        Stefan

Re: Gnus using lexical-binding

[Andy Moreton]

On Sat 30 Jan 2021, Stefan Monnier wrote:

>> Fails starting gnus for me. Setting `debug-on-error' gives backtrace:
>>
>> Debugger entered--Lisp error: (void-variable gnus-tmp-level)
>
> Thanks.  EMACS_DYNVARS_FILE caught this one earlier today as well, indeed.

A little further, but:

Debugger entered--Lisp error: (void-variable number)
  (eq number t)
  (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr (assq 'dormant gnus-tmp-marked))) (gnus-range-length (cdr (assq 'tick gnus-tmp-marked)))))) (t number))
  (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr (assq ... gnus-tmp-marked))) (gnus-range-length (cdr (assq ... gnus-tmp-marked)))))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)
  (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr ...)) (gnus-range-length (cdr ...))))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live))
  (let (gnus-position) (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length ...) (gnus-range-length ...)))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)) (setq gnus-position (point)) (insert (format "%-55s" (let* ((val (eval (if ... ... gnus-tmp-qualified-group))) (need (- 55 (string-width val)))) (if (> need 0) (concat nil val (make-string need 32)) val)))) (insert-char 32 (max (- 75 (current-column)) 0)) (add-text-properties (point) (progn (insert gnus-tmp-news-server) (point)) (cons 'face (cons (list 'italic 'default) '(gnus-face t)))) (insert "\n") (if gnus-position (put-text-property gnus-position 
 (1+ gnus-position) 'gnus-position t)))
  eval((let (gnus-position) (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number ... ...))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)) (setq gnus-position (point)) (insert (format "%-55s" (let* ((val (eval ...)) (need (- 55 ...))) (if (> need 0) (concat nil val (make-string need 32)) val)))) (insert-char 32 (max (- 75 (current-column)) 0)) (add-text-properties (point) (progn (insert gnus-tmp-news-server) (point)) (cons 'face (cons (list 'italic 'default) '(gnus-face t)))) (insert "\n") (if gnus-position (put-text-property gnus-position (1+ gnus-position) 'gnus-position t))) t)
  gnus-group-insert-group-line("dummy.group" 0 nil 0 nil)
  gnus-update-group-mark-positions()
  gnus-group-mode()
  gnus-group-setup-buffer()
  gnus-group-list-groups(nil)
  #f(compiled-function () #<bytecode 0x991f1d58b065dda>)()
  gnus-1(nil nil nil)
  gnus(nil)
  funcall-interactively(gnus nil)
  call-interactively(gnus record nil)
  command-execute(gnus record)
  execute-extended-command(nil "gnus" nil)
  funcall-interactively(execute-extended-command nil "gnus" nil)
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

Re: Gnus using lexical-binding

[Stefan Monnier]

> Debugger entered--Lisp error: (void-variable number)
>   (eq number t)

Aha!

>   (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number

That seems to be coming from `gnus-group-line-format-alist` where "all"
the vars it refers have names that start with `gnus-tmp-` except for
`number`, apparently.

I added a patch to accommodate for that.
I hope there aren't many more of those: they're hard to find.


        Stefan

Re: Gnus using lexical-binding

[Andy Moreton]

On Sat 30 Jan 2021, Stefan Monnier wrote:

>> Debugger entered--Lisp error: (void-variable number)
>>   (eq number t)
>
> Aha!
>
>>   (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number
>
> That seems to be coming from `gnus-group-line-format-alist` where "all"
> the vars it refers have names that start with `gnus-tmp-` except for
> `number`, apparently.
>
> I added a patch to accommodate for that.
> I hope there aren't many more of those: they're hard to find.

Thats gets a little further, but...

Debugger entered--Lisp error: (void-variable gnus-tmp-number-total)
  (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr (assq ... gnus-tmp-marked))) (gnus-range-length (cdr (assq ... gnus-tmp-marked)))))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)
  (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length (cdr ...)) (gnus-range-length (cdr ...))))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live))
  (let (gnus-position) (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number (gnus-range-length ...) (gnus-range-length ...)))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)) (setq gnus-position (point)) (insert (format "%-55s" (let* ((val (eval (if ... ... gnus-tmp-qualified-group))) (need (- 55 (string-width val)))) (if (> need 0) (concat nil val (make-string need 32)) val)))) (insert-char 32 (max (- 75 (current-column)) 0)) (add-text-properties (point) (progn (insert gnus-tmp-news-server) (point)) (cons 'face (cons (list 'italic 'default) '(gnus-face t)))) (insert "\n") (if gnus-position (put-text-property gnus-position 
 (1+ gnus-position) 'gnus-position t)))
  eval((let (gnus-position) (insert (format "%c%c%d%c %6s %7d %s%c" gnus-tmp-marked-mark gnus-tmp-subscribed gnus-tmp-level gnus-tmp-process-marked (cond ((eq number t) "*") ((numberp number) (int-to-string (+ number ... ...))) (t number)) gnus-tmp-number-total gnus-group-indentation gnus-tmp-summary-live)) (setq gnus-position (point)) (insert (format "%-55s" (let* ((val (eval ...)) (need (- 55 ...))) (if (> need 0) (concat nil val (make-string need 32)) val)))) (insert-char 32 (max (- 75 (current-column)) 0)) (add-text-properties (point) (progn (insert gnus-tmp-news-server) (point)) (cons 'face (cons (list 'italic 'default) '(gnus-face t)))) (insert "\n") (if gnus-position (put-text-property gnus-position (1+ gnus-position) 'gnus-position t))) t)
  gnus-group-insert-group-line("dummy.group" 0 nil 0 nil)
  gnus-update-group-mark-positions()
  gnus-group-mode()
  gnus-group-setup-buffer()
  gnus-group-list-groups(nil)
  #f(compiled-function () #<bytecode 0x991f1d2aaa768d6>)()
  gnus-1(nil nil nil)
  gnus(nil)
  funcall-interactively(gnus nil)
  call-interactively(gnus record nil)
  command-execute(gnus record)
  execute-extended-command(nil "gnus" nil)
  funcall-interactively(execute-extended-command nil "gnus" nil)
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

Re: Gnus using lexical-binding

[Stefan Monnier]

> Thats gets a little further, but...
>
> Debugger entered--Lisp error: (void-variable gnus-tmp-number-total)

OK, I `defvar`d all the gnus-tmp-* I could find now.
Please try again.

Note that the branch has a new name `scratch/lexical-gnus-rc` because
I have now cleaned up the commit messages, so it's basically "ready to
be merged".


        Stefan

Re: Gnus using lexical-binding

[Andy Moreton]

On Sat 30 Jan 2021, Stefan Monnier wrote:

>> Thats gets a little further, but...
>>
>> Debugger entered--Lisp error: (void-variable gnus-tmp-number-total)
>
> OK, I `defvar`d all the gnus-tmp-* I could find now.
> Please try again.
>
> Note that the branch has a new name `scratch/lexical-gnus-rc` because
> I have now cleaned up the commit messages, so it's basically "ready to
> be merged".

After a full bootstrap this branch now seems to have a working gnus, at
least for nntp (I don't use gnus for email).


    AndyM

Re: Gnus using lexical-binding

[David Engster]

> And please, after testing it, tell me how it went, regardless if you
> encountered problems or not.

I've been running this now for a few hours without problems.

-David

Re: Gnus using lexical-binding

[Basil L. Contovounesios]

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

>> Thats gets a little further, but...
>>
>> Debugger entered--Lisp error: (void-variable gnus-tmp-number-total)
>
> OK, I `defvar`d all the gnus-tmp-* I could find now.
> Please try again.
>
> Note that the branch has a new name `scratch/lexical-gnus-rc` because
> I have now cleaned up the commit messages, so it's basically "ready to
> be merged".

Seems to be working fine for me, thanks,

-- 
Basil

Re: Gnus using lexical-binding

[Stefan Monnier]

Thanks Lars, David, Andy, and Basil,

It's clearly not enough testing for a release, but I decided it was
enough for now, I pushed the new code to `master`.


        Stefan

Re: Gnus using lexical-binding

[Eric Abrahamsen]

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

>> Thats gets a little further, but...
>>
>> Debugger entered--Lisp error: (void-variable gnus-tmp-number-total)
>
> OK, I `defvar`d all the gnus-tmp-* I could find now.
> Please try again.
>
> Note that the branch has a new name `scratch/lexical-gnus-rc` because
> I have now cleaned up the commit messages, so it's basically "ready to
> be merged".

I'm now seeing a bug with `gnus-article-browse-html-article' that might
(?) be a result of this, I'm not sure. Below is the definition of
`gnus-visible-headers', which I haven't changed from its default. When
viewing an HTML part, however, and we reach the function
`article-hide-headers', the definition of `gnus-visible-headers' now
looks like this:

((funcall #'#f(compiled-function () #<bytecode 0x134a5ff509cc6ffd>)))

Which `article-hide-headers' isn't set up to handle.

Could this be a result of lexical binding?


(defcustom gnus-visible-headers
  "^From:\\|^Newsgroups:\\|^Subject:\\|^Date:\\|^Followup-To:\\|^Reply-To:\\|^Organization:\\|^Summary:\\|^Keywords:\\|^To:\\|^[BGF]?Cc:\\|^Posted-To:\\|^Mail-Copies-To:\\|^Mail-Followup-To:\\|^Apparently-To:\\|^Gnus-Warning:\\|^Resent-From:"
  "All headers that do not match this regexp will be hidden.
This variable can also be a list of regexp of headers to remain visible.
If this variable is non-nil, `gnus-ignored-headers' will be ignored."
  :type '(choice
	  (repeat :value-to-internal (lambda (widget value)
				       (custom-split-regexp-maybe value))
		  :match (lambda (widget value)
			   (or (stringp value)
			       (widget-editable-list-match widget value)))
		  regexp)
	  (const :tag "Use gnus-ignored-headers" nil)
	  regexp)
  :group 'gnus-article-hiding)

Re: Gnus using lexical-binding

[Stefan Monnier]

> I'm now seeing a bug with `gnus-article-browse-html-article' that might
> (?) be a result of this, I'm not sure.

It's triggered by the recent change, indeed.
It's due to a longer standing bug, tho.
The problem is in these two chunks of code in `gnus-art.el`:

		 (let ((gnus-visible-headers
			(or (get 'gnus-visible-headers 'standard-value)
			    gnus-visible-headers)))
		   (article-hide-headers))

and

    (let ((gnus-visible-headers (or (get 'gnus-visible-headers 'standard-value)
				    gnus-visible-headers))
	  (gnus-mime-display-attachment-buttons-in-header nil)
	  ;; As we insert a <hr>, there's no need for the body boundary.
	  (gnus-treat-body-boundary nil))
      (gnus-summary-show-article)))

I'm not completely sure why this code looks up the `standard-value`, but
the `standard-value` property is supposed to contain a cons cell whose
car is an *expression* that needs to be evaluated to get the variable's
intended standard value.

So it will never be nil and using it as a value is just wrong.

I installed a patch into `master` which should hopefully fix
the problem.


        Stefan

Re: Gnus using lexical-binding

[Eric Abrahamsen]

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

>> I'm now seeing a bug with `gnus-article-browse-html-article' that might
>> (?) be a result of this, I'm not sure.
>
> It's triggered by the recent change, indeed.
> It's due to a longer standing bug, tho.

[...]

> I'm not completely sure why this code looks up the `standard-value`, but
> the `standard-value` property is supposed to contain a cons cell whose
> car is an *expression* that needs to be evaluated to get the variable's
> intended standard value.

Ah, hence the lambda. I can't immediately think of why we'd need the
standard value of the option here, either.

> So it will never be nil and using it as a value is just wrong.
>
> I installed a patch into `master` which should hopefully fix
> the problem.

Looks good! Thanks for the quick fix.

Eric

Re: Gnus using lexical-binding

[Ted Zlatanov]

Hi!

I got a probably related error after restarting Gnus, which had been
running for a while. Sorry for the late catch. I have a custom topic
function `gnus-user-format-function-topic-line' that started
complaining:

Debugger entered--Lisp error: (void-variable total-number-of-articles)
  (= 0 total-number-of-articles)
  (if (= 0 total-number-of-articles) 'font-lock-comment-face 'font-lock-function-name-face)
  (let* ((topic-face (if (= 0 total-number-of-articles) 'font-lock-comment-face 'font-lock-function-name-face)) (h (tzz-frame-font-size))) (propertize (format "%s %s" name (if (and tzz-ungradients (image-type-available-p 'svg)) (tzz-image-from-svg-string (+ 4 h) h total-number-of-articles nil) (tzz-summarize-number total-number-of-articles))) 'face topic-face))
  gnus-user-format-function-topic-line(nil)
  (format "%s[ %s ] %s\n" indentation (gnus-user-format-function-topic-line gnus-tmp-header) visible)
  (insert (format "%s[ %s ] %s\n" indentation (gnus-user-format-function-topic-line gnus-tmp-header) visible))
  eval((insert (format "%s[ %s ] %s\n" indentation (gnus-user-format-function-topic-line gnus-tmp-header) visible)) ((indentation . "    ") (visible . "") (name . "comics") (level . 2) (number-of-groups . 1) (total-number-of-articles . 1) (entries (1 ("nntp+news.gwene.org:gwene.com.xkcd" 3 ((1 . 1664)) ((unexist) (seen (1 . 417) (423 . 445) (449 . 453) (455 . 464) 468 (507 . 511) (513 . 521) (525 . 536) 538 (540 . 544) (546 . 554) (558 . 580) (584 . 589) (605 . 622) (636 . 651) (653 . 655) (659 . 660) (662 . 664) (668 . 684) (686 . 705) (707 . 708) (710 . 711) (714 . 733) 736 (739 . 749) (751 . 752) (757 . 761) (764 . 765) 771 773 (776 . 920) (922 . 929) (931 . 937) (940 . 941) (943 . 961) (963 . 974) 979 (984 . 996) (998 . 1044) (1047 . 1054) (1056 . 1065) (1076 . 1078) (1082 . 1085) (109
 1 . 1093) (1095 . 1104) (1106 . 1120) (1122 . 1139) (1142 . 1144) (1148 . 1149) ...) (forward 164 411) (score (778 . 1000) (63 . 1000))) "news.gwene.org")))))
  gnus-topic-insert-topic-line("comics" t t 2 ((1 ("nntp+news.gwene.org:gwene.com.xkcd" 3 ((1 . 1664)) ((unexist) (seen (1 . 417) (423 . 445) (449 . 453) (455 . 464) 468 (507 . 511) (513 . 521) (525 . 536) 538 (540 . 544) (546 . 554) (558 . 580) (584 . 589) (605 . 622) (636 . 651) (653 . 655) (659 . 660) (662 . 664) (668 . 684) (686 . 705) (707 . 708) (710 . 711) (714 . 733) 736 (739 . 749) (751 . 752) (757 . 761) (764 . 765) 771 773 (776 . 920) (922 . 929) (931 . 937) (940 . 941) (943 . 961) (963 . 974) 979 (984 . 996) (998 . 1044) (1047 . 1054) (1056 . 1065) (1076 . 1078) (1082 . 1085) (1091 . 1093) (1095 . 1104) (1106 . 1120) (1122 . 1139) (1142 . 1144) (1148 . 1149) ...) (forward 164 411) (score (778 . 1000) (63 . 1000))) "news.gwene.org"))) 1)
  gnus-topic-prepare-topic((("comics" visible nil nil)) 2 5 nil nil 1 nil)

The confusing thing here is that the eval lexical environment is
correct. I'm not sure what's happening in the stack. I can reproduce it
with a shorter example:

(defun gnus-user-format-function-topic-line (dummy)
  (message "total %s" total-number-of-articles))

(let ((gnus-topic-line-format "%i[ %u&topic-line; ] %v\n"))
     (gnus-update-format-specifications nil 'topic))

(let ((gnus-tmp-header nil))
     (eval gnus-topic-line-format-spec
           '((indentation . "    ") (visible . "") (name . "comics") (level . 2) (number-of-groups . 1) (total-number-of-articles . 1))))

I hope that helps track it down.

Thank you
Ted

Re: Gnus using lexical-binding

[Stefan Monnier]

> Debugger entered--Lisp error: (void-variable total-number-of-articles)
>   (= 0 total-number-of-articles)
>   (if (= 0 total-number-of-articles) 'font-lock-comment-face 'font-lock-function-name-face)
>   (let* ((topic-face (if (= 0 total-number-of-articles) 'font-lock-comment-face 'font-lock-function-name-face)) (h (tzz-frame-font-size))) (propertize (format "%s %s" name (if (and tzz-ungradients (image-type-available-p 'svg)) (tzz-image-from-svg-string (+ 4 h) h total-number-of-articles nil) (tzz-summarize-number total-number-of-articles))) 'face topic-face))
>   gnus-user-format-function-topic-line(nil)
>   (format "%s[ %s ] %s\n" indentation (gnus-user-format-function-topic-line gnus-tmp-header) visible)
>   (insert (format "%s[ %s ] %s\n" indentation (gnus-user-format-function-topic-line gnus-tmp-header) visible))
>   eval((insert (format "%s[ %s ] %s\n" indentation
>   (gnus-user-format-function-topic-line gnus-tmp-header) visible))
>   ((indentation . "    ") (visible . "") (name . "comics") (level . 2)
>   (number-of-groups . 1) (total-number-of-articles . 1) (entries (1

That sucks!

> The confusing thing here is that the eval lexical environment
> is correct.

It's correct but it's lexical, so those vars can't be seen from functions
called from `gnus-topic-line-format-spec`.

I installed the patch below which should sadly fix it,


        Stefan


Summary: * lisp/gnus/gnus-topic.el: Fix a backward incompatibility

(gnus-topic-insert-topic-line): Make the vars used in
`gnus-topic-line-format-spec` dynamically scoped since it seems
that they're sometimes accessed from functions called by
`gnus-topic-line-format-spec` :-(

* lisp/gnus/gnus-util.el (gnus--\,@): Move macro to here...
* lisp/gnus/gnus-art.el (gnus--\,@): .. from here.

* lisp/gnus/gnus.el (gnus-method-to-server): Apply DeMorgan.


diff --git a/lisp/gnus/gnus-art.el b/lisp/gnus/gnus-art.el
index 7ded9e40e9..c9afa3ac94 100644
--- a/lisp/gnus/gnus-art.el
+++ b/lisp/gnus/gnus-art.el
@@ -4325,10 +4325,6 @@ article-verify-cancel-lock
   (if (gnus-buffer-live-p gnus-original-article-buffer)
       (canlock-verify gnus-original-article-buffer)))
 
-(defmacro gnus--\,@ (exp)
-  (declare (debug t))
-  `(progn ,@(eval exp t)))
-
 (gnus--\,@
  (mapcar (lambda (func)
            `(defun ,(intern (format "gnus-%s" func))
diff --git a/lisp/gnus/gnus-topic.el b/lisp/gnus/gnus-topic.el
index e7d1cf8616..3253b7853d 100644
--- a/lisp/gnus/gnus-topic.el
+++ b/lisp/gnus/gnus-topic.el
@@ -627,7 +627,14 @@ gnus-tmp-header
 
 (defun gnus-topic-insert-topic-line (name visiblep shownp level entries
 					  &optional unread)
+  (gnus--\,@
+   (let ((vars '(indentation visible name level number-of-groups
+                 total-number-of-articles entries)))
+     `((with-suppressed-warnings ((lexical ,@vars))
+         ,@(mapcar (lambda (s) `(defvar ,s)) vars)))))
   (let* ((visible (if visiblep "" "..."))
+	 (level level)
+	 (name name)
 	 (indentation (make-string (* gnus-topic-indent-level level) ? ))
 	 (total-number-of-articles unread)
 	 (number-of-groups (length entries))
@@ -640,14 +647,7 @@ gnus-topic-insert-topic-line
 	(add-text-properties
 	 (point)
 	 (prog1 (1+ (point))
-	   (eval gnus-topic-line-format-spec
-                 `((indentation . ,indentation)
-                   (visible . ,visible)
-                   (name . ,name)
-                   (level . ,level)
-                   (number-of-groups . ,number-of-groups)
-                   (total-number-of-articles . ,total-number-of-articles)
-                   (entries . ,entries))))
+	   (eval gnus-topic-line-format-spec t))
 	 (list 'gnus-topic name
 	       'gnus-topic-level level
 	       'gnus-topic-unread unread
diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index 3c7c948c2b..f80243cfed 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -1068,6 +1068,11 @@ gnus-run-mode-hooks
 
 ;;; Various
 
+(defmacro gnus--\,@ (exp)
+  "Splice EXP's value (a list of Lisp forms) into the code."
+  (declare (debug t))
+  `(progn ,@(eval exp t)))
+
 (defvar gnus-group-buffer)		; Compiler directive
 (defun gnus-alive-p ()
   "Say whether Gnus is running or not."
diff --git a/lisp/gnus/gnus.el b/lisp/gnus/gnus.el
index 98664ac2b4..7b94c64ae7 100644
--- a/lisp/gnus/gnus.el
+++ b/lisp/gnus/gnus.el
@@ -3212,9 +3212,9 @@ gnus-method-to-server
 		     (format "%s" (car method))
 		   (format "%s:%s" (car method) (cadr method))))
 	   (name-method (cons name method)))
-      (when (and (not no-enter-cache)
-		 (not (member name-method gnus-server-method-cache))
-		 (not (assoc (car name-method) gnus-server-method-cache)))
+      (unless (or no-enter-cache
+		  (member name-method gnus-server-method-cache)
+		  (assoc (car name-method) gnus-server-method-cache))
 	(push name-method gnus-server-method-cache))
       name)))
 

Re: Gnus using lexical-binding

[Ted Zlatanov]

On Mon, 08 Feb 2021 19:10:48 -0500 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

>> The confusing thing here is that the eval lexical environment
>> is correct.

SM> It's correct but it's lexical, so those vars can't be seen from functions
SM> called from `gnus-topic-line-format-spec`.

I looked through (info "(elisp) Lexical Binding") and I think I
understand the issue better. Thank you for fixing it. I confirmed it's
all clear.

SM> I installed the patch below which should sadly fix it,

I have some (probably dumb) suggestions.

The fix and the whole pre-existing Gnus "user defines special function"
mechanism feels very magical. Could we instead define a standard way of
calling these custom user-defined functions in Gnus, passing them an
explicit alist or plist of special properties? That would also make it
easier from within the function to check what's passed down, and
probably remove some complex code in the long run.

The code that calls them today could check their arity and if it's 1
(current call signature), warn and do the workaround. If it's 2, pass
the alist or plist with no workarounds.

Another way may be to provide a special macro to define these functions
in a backwards-compatible way. If they have not been defined with the
macro, warn and use the workaround. Otherwise, pass them the special
values.

...and yet another way may be a new escape char in the format spec for
the new style functions with 2 arguments. Users can switch to it when
they are ready, and the old escape char gets deprecated.

I hope that's useful.
Ted

Re: Gnus using lexical-binding

[Stefan Monnier]

> The fix and the whole pre-existing Gnus "user defines special function"
> mechanism feels very magical.

Amen.

> Could we instead define a standard way of calling these custom
> user-defined functions in Gnus, passing them an explicit alist or
> plist of special properties?

I'd welcome such a change,


        Stefan

Re: Gnus using lexical-binding

[Lars Ingebrigtsen]

Ted Zlatanov <tzz@lifelogs.com> writes:

> The code that calls them today could check their arity and if it's 1
> (current call signature), warn and do the workaround. If it's 2, pass
> the alist or plist with no workarounds.

Sounds good to me.  Perhaps a hash table with properties?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no