Why is Elisp's defvar weird? And is eval_sub broken?

Why is Elisp's defvar weird? And is eval_sub broken?

[Kelly Dean]

desktop.el has these top-level definitions:
(defvar desktop-first-buffer)
(defvar desktop-buffer-ok-count)
(defvar desktop-buffer-fail-count)

The docstring for defvar says:
⌜Define SYMBOL as a variable, and return SYMBOL.
...
The `defvar' form also declares the variable as "special",
so that it is always dynamically bound even if `lexical-binding' is t.
...
If INITVALUE is missing, SYMBOL's value is not set.

If SYMBOL has a local binding, then this form affects the local
binding.⌝

But that's wrong. If INITVALUE is missing, and lexical-binding is t (as is the case in desktop.el), then not only is the value not set, but also the variable is _not_ declared special, even if the defvar is at top level.

That means that even after loading desktop.el, if you let-bind the three variables above in a function defined in a file other than desktop.el, and lexical-binding is t in that other file, then those variables will be bound lexically, not dynamically.

This was causing an inexplicable bug in my code that uses functions from desktop.el, that I could figure out how to fix only by using setq instead of «let» for those variables. Then today I happened to read the source code for defvar and discovered what's really going on. I can fix my bug without setq, by instead using defvar (without INITVALUE) before «let». If the docstring for defvar were true (i.e. top-level defvar declares special (even if INITVALUE is missing)), then I wouldn't need to use defvar before the «let», because desktop.el would have already made those variables special. It's no problem for me to use defvar before the «let», but the docstring should say what defvar really does, so people know it's necessary in this case.

Also, CL doesn't behave this way. E.g. In Elisp (in Emacs 24.4):
(setq lexical-binding t)
(let ((foo 'bar)) (defvar foo) foo) → bar
(let ((foo 'baz)) (makunbound 'foo) foo) → baz

But in CL:
(let ((foo 'bar)) (defvar foo) foo) → bar
(let ((foo 'baz)) (makunbound 'foo) foo) → error: foo is unbound

In Elisp, both let-bindings are lexical, but in CL, the second let-binding is dynamic.

What's the purpose of Elisp behaving this way? Is it just to enable local use of dynamic variables (for implicit arguments and return values of functions) without having to clutter the globals with otherwise-unneeded special variables? If so, a cleaner way to do it would be with a dynamic-let special form, rather than a weirdly-behaving defvar.

Also, in Elisp:
(setq lexical-binding t)
(let ((foo 0))
  (defvar foo)
  (let ((foo 1))
    foo)) → 0

That's because eval_sub in eval.c looks up the variable in the lexical environment using only Fassq, without first using Fmemq to check for a local dynamic binding. Is that behavior actually correct?

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Stefan Monnier]

> But that's wrong. If INITVALUE is missing, and lexical-binding is
>  t (as is the case in desktop.el), then not only is the value not set,
>  but also the variable is _not_ declared special, even if the defvar
>  is at top level.

The declaration of the var as being dynamically-scoped (aka "special")
is *local* to the (rest of the) current scope (typically the current file).

This is indispensable so that one package can use a dynamically-bound
variable `foo' without breaking some other package that expects `foo' to
be lexically-bound.  Normally, such conflicts should never happen
because all special vars should be named with a "package prefix", but
sadly, reality is different, so it was indispensable to make this
effect local, to allow lexical-binding code to work reliably.

> That means that even after loading desktop.el, if you let-bind the
> three variables above in a function defined in a file other than
> desktop.el, and lexical-binding is t in that other file, then those
> variables will be bound lexically, not dynamically.

That's right.
If you're lucky (more specifically, if you only let-bind those vars but
you don't use them locally), the byte-compiler will emit a warning that
those let-bindings aren't used (which is usually a sign that you need
to add a (defvar <foo>) earlier in the file).

> That's because eval_sub in eval.c looks up the variable in the lexical
> environment using only Fassq, without first using Fmemq to check for
> a local dynamic binding.  Is that behavior actually correct?

I wouldn't argue it's correct, but I'd rather not pay the price of an
additional memq check to cater to such brain-dead misuse of defvar.
Arguably, the byte-compiler should flag such misuse, tho currently it
misses it (tho it does catch the case of:

   (let ((my-foo 0))
     (let ((my-foo 1))
       my-foo))
   
   (defvar my-foo)


-- Stefan

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Kelly Dean]

Stefan Monnier wrote:
> The declaration of the var as being dynamically-scoped (aka "special")
> is *local* to the (rest of the) current scope (typically the current file).
>
> This is indispensable so that one package can use a dynamically-bound
> variable `foo' without breaking some other package that expects `foo' to
> be lexically-bound.

desktop.el doesn't use desktop-first-buffer, desktop-buffer-ok-count, and desktop-buffer-fail-count as global variables. It only uses them as dynamic variables, for implicit arguments and return values of functions. I.e. outside of a let-binding, it doesn't use the symbols (except in defvar, just to avoid byte compiler warnings).

For that case, what I had in mind was a dynamic-let special form, being the same as «let», except with the following case removed:
if (!NILP (lexenv) && SYMBOLP (var)
	 && !XSYMBOL (var)->declared_special
	 && NILP (Fmemq (var, Vinternal_interpreter_environment)))
  lexenv = Fcons (Fcons (var, tem), lexenv);

so it always just does specbind (var, tem). The byte compiler could flag an error if you use dynamic-let on a symbol that's already used as a lexical variable. This way, there's no need to use defvar on symbols such as the desktop.el symbols above. So defvar can be reserved just for the symbols that really are special, i.e. the ones that are used as global variables and for which dynamic variables instead of lexical variables are created to shadow the globals when you let-bind the symbols using standard «let» (in any file).

A declaration of free dynamic variables for a function could tell the byte compiler that those free variables aren't typos, since otherwise the byte compiler would expect either the symbol to be declared special or a lexical variable by that name to be in scope.

Also have a lexical-let special form (replacing the old lexical-let macro), being the same as «let», except with case above replaced by:
CHECK_SYMBOL (var);
if (NILP (lexenv)) lexenv = Qt;
lexenv = Fcons (Fcons (var, tem), lexenv);

and have it never do specbind (var, tem). The byte compiler could give a warning if you use lexical-let on a symbol that's declared special. Then everybody could use this faster form in the cases where all the variables being bound in the «let» are supposed to be lexical. This also has the advantage of ensuring that those variables _are_ bound lexically even if you forgot that some of the symbols you're using were declared special, and the byte compiler can see your intent to use lexicals and warn you about the specials, which helps catch bugs.

Then you never need standard (and slow, and bug-prone) «let», except for compatibility with old code, and for the rare cases where you might need it for some mind-bending macros.

I would vote for ⌜dlet⌝ and ⌜llet⌝ as the names, or at least ⌜dynlet⌝ and ⌜lexlet⌝, since Elisp code is already too verbose.

For both correctness and speed, have the standard llet (accessible to interpreted code) include the CHECK_SYMBOL (var) above, and have the standard dlet include a runtime check of shadowing a lexical. Have byte-compiled code use a pair of optimized special forms, not accessible to interpreted code, with those checks omitted, since they're done at compile time.

> Normally, such conflicts should never happen
> because all special vars should be named with a "package prefix", but
> sadly, reality is different, so it was indispensable to make this
> effect local, to allow lexical-binding code to work reliably.

By using llet, the byte compiler will catch such conflicts, and your code (interpreted or compiled) that uses it will work reliably despite the conflicts. This means defvar's weird behavior is no longer needed.

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Stefan Monnier]

> A declaration of free dynamic variables for a function could tell the
> byte compiler that those free variables aren't typos, since
> otherwise the byte compiler would expect either the symbol to be
> declared special or a lexical variable by that name to be in scope.

There is such a declaration already.  It's called (defvar <foo>).  Tada!

>> Normally, such conflicts should never happen
>> because all special vars should be named with a "package prefix", but
>> sadly, reality is different, so it was indispensable to make this
>> effect local, to allow lexical-binding code to work reliably.
> By using llet, the byte compiler will catch such conflicts, and your code
> (interpreted or compiled) that uses it will work reliably despite the
> conflicts. This means defvar's weird behavior is no longer needed.

It's not weird once you understand that it's a compiler directive which
is absent from the .elc file.


        Stefan

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Daniel Colascione]

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

On 02/14/2015 06:36 AM, Stefan Monnier wrote:
>> A declaration of free dynamic variables for a function could tell the
>> byte compiler that those free variables aren't typos, since
>> otherwise the byte compiler would expect either the symbol to be
>> declared special or a lexical variable by that name to be in scope.
> 
> There is such a declaration already.  It's called (defvar <foo>).  Tada!
> 
>>> Normally, such conflicts should never happen
>>> because all special vars should be named with a "package prefix", but
>>> sadly, reality is different, so it was indispensable to make this
>>> effect local, to allow lexical-binding code to work reliably.
>> By using llet, the byte compiler will catch such conflicts, and your code
>> (interpreted or compiled) that uses it will work reliably despite the
>> conflicts. This means defvar's weird behavior is no longer needed.
> 
> It's not weird once you understand that it's a compiler directive which
> is absent from the .elc file.

How are users supposed to understand that when it's completely absent
from the documentation?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Kelly Dean]

Stefan Monnier wrote:
>>> Normally, such conflicts should never happen
>>> because all special vars should be named with a "package prefix", but
>>> sadly, reality is different, so it was indispensable to make this
>>> effect local, to allow lexical-binding code to work reliably.
>> By using llet, the byte compiler will catch such conflicts, and your code
>> (interpreted or compiled) that uses it will work reliably despite the
>> conflicts. This means defvar's weird behavior is no longer needed.
>
> It's not weird once you understand that it's a compiler directive which
> is absent from the .elc file.

If Elisp's defvar with no init value is used, then it avoids conflict, but the packages that are a problem in the first place (by declaring non-package-prefixed symbols special) will also be ones that tend to use defvar _with_ an init value (so the symbols really are declared special, and their defvars are in the elc). Elisp's defvar doesn't prevent conflict in that case, so it seems the local-specialness feature doesn't fulfill its intended purpose.

In contrast, llet would prevent conflict in that case.

(Global) specialness ambushes code that uses Lisp's standard «let», because «let» lets outside code decide how to bind the symbols. Elisp's local specialness just lets the outside code decline to launch the ambush. In contrast, llet ensures that the ambush will fail. That's why it's the better solution. And it happens to be faster.

And dlet is a faster way of doing what you can currently do in Elisp using defvar (with no init value) followed by standard «let». Except dlet causes only local dynamicness (called functions can read/set the variables it binds), which is all it needs, not local specialness (makes following «let»s bind dynamically) like Elisp's defvar does.

>> A declaration of free dynamic variables for a function could tell the
>> byte compiler that those free variables aren't typos, since
>> otherwise the byte compiler would expect either the symbol to be
>> declared special or a lexical variable by that name to be in scope.
>
> There is such a declaration already.  It's called (defvar <foo>).  Tada!

Yes, though using defvar for this declaration in the function also causes local specialness, when all that's needed in this case is just to tell the byte compiler that the variables aren't mistakes.

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Stefan Monnier]

> If Elisp's defvar with no init value is used, then it avoids conflict, but
> the packages that are a problem in the first place (by declaring
> non-package-prefixed symbols special) will also be ones that tend to use
> defvar _with_ an init value (so the symbols really are declared special, and
> their defvars are in the elc). Elisp's defvar doesn't prevent conflict in
> that case, so it seems the local-specialness feature doesn't fulfill its
> intended purpose.

That's indeed a risk, but it's one that's a lot easier to manage.

> In contrast, llet would prevent conflict in that case.

Right, you can introduce llet and llet* and then some more l<foos> for
macros that expand to uses of `let' or `let*' and ....

Or you can take a page from Common-Lisp's book and run with it.

I chose the second option.  It's not perfect, but so far I don't regret it.

> (Global) specialness ambushes code that uses Lisp's standard «let», because
> «let» lets outside code decide how to bind the symbols.

That's true, which is why I added the annoying warnings to `defvar' when
the symbol seems not to use a prefix.

> And it happens to be faster.

It should make 0 difference to byte-compiled code and I don't care much
about the speed of the non-byte-compiled code.

> And dlet is a faster way of doing what you can currently do in Elisp using
> defvar (with no init value) followed by standard «let».

Again: no speed difference in byte-compiled code.

>> There is such a declaration already.  It's called (defvar <foo>).  Tada!
> Yes, though using defvar for this declaration in the function also causes
> local specialness, when all that's needed in this case is just to tell the
> byte compiler that the variables aren't mistakes.

Using the same identifiers sometimes as a lexical var and sometimes as
a dynamic var is evil for the poor human reader.  Si I have no intention
to try and refine the semantics of such cases.


        Stefan

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Kelly Dean]

Stefan Monnier wrote:
> Using the same identifiers sometimes as a lexical var and sometimes as
> a dynamic var is evil for the poor human reader.  Si I have no intention
> to try and refine the semantics of such cases.

So the local-specialness feature is intended exclusively to be a workaround for the problem of non-prefixed symbols, not intended for any other purpose?

In that case, should desktop-first-buffer, desktop-buffer-ok-count, and desktop-buffer-fail-count be given init values to prevent local specialness, so that code outside desktop.el that calls desktop functions that use those variables doesn't have to do defvar on them?

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Stefan Monnier]

> So the local-specialness feature is intended exclusively to be a workaround
> for the problem of non-prefixed symbols, not intended for any other purpose?

It's mostly intended so that someone can use a dynamically scoped
variable without worrying about stepping on other people's toes, yes.

> In that case, should desktop-first-buffer, desktop-buffer-ok-count, and
> desktop-buffer-fail-count be given init values to prevent local specialness,
> so that code outside desktop.el that calls desktop functions that use those
> variables doesn't have to do defvar on them?

We could, but AFAICT, these vars are only used in desktop.el (they
could/should use a double-dash to indicate that they're internal), so it
would be kind of pointless.


        Stefan

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Kelly Dean]

Stefan Monnier wrote:
>> In that case, should desktop-first-buffer, desktop-buffer-ok-count, and
>> desktop-buffer-fail-count be given init values to prevent local specialness,
>> so that code outside desktop.el that calls desktop functions that use those
>> variables doesn't have to do defvar on them?
>
> We could, but AFAICT, these vars are only used in desktop.el (they
> could/should use a double-dash to indicate that they're internal), so it
> would be kind of pointless.

If they're locally special, then I have to declare them in my code too, before I let-bind them so I can call desktop-create-buffer (which barfs if I don't), even though I don't use them.

For my code, see line 942 (and 97 for the macro that generates the declarations) of:
http://prtime.org/emacs/usablizer.el

That's no problem, but I wanted to make sure you really do want variables to have to be re-declared like that.

This is where I was previously just doing setq (followed by makunbound after calling desktop-create-buffer), until I discovered that I can do defvar followed by «let».

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Stefan Monnier]

> If they're locally special, then I have to declare them in my code too,
> before I let-bind them so I can call desktop-create-buffer (which barfs if
> I don't), even though I don't use them.

> For my code, see line 942 (and 97 for the macro that generates the declarations) of:
> http://prtime.org/emacs/usablizer.el

> That's no problem, but I wanted to make sure you really do want variables to
> have to be re-declared like that.

Your code is clearly meant to be inside desktop.el, so it's perfectly
normal to have to add various extra declarations to reproduce some
context before calling internal functions.

> This is where I was previously just doing setq (followed by makunbound after
> calling desktop-create-buffer), until I discovered that I can do defvar
> followed by «let».

A general rule is that if you need makunbound you're probably doing
something wrong.


        Stefan

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Kelly Dean]

Stefan Monnier wrote:
> Your code is clearly meant to be inside desktop.el, so it's perfectly
> normal to have to add various extra declarations to reproduce some
> context before calling internal functions.

Should I add the closed-buffer tracker to desktop.el?

Re: Why is Elisp's defvar weird? And is eval_sub broken?

[Stefan Monnier]

>> Your code is clearly meant to be inside desktop.el, so it's perfectly
>> normal to have to add various extra declarations to reproduce some
>> context before calling internal functions.
> Should I add the closed-buffer tracker to desktop.el?

I haven't looked at it and I don't use desktop.el (nor do I hack on it
very much), so I don't really know.  But since you seem to think that
it's a feature that should be there, I suggest you submit it for
inclusion as a patch.

My comment was just meaning that it's normal to have to add such
declarations when you're meddling in the internals of a package.  Even if
this meddling is bound to stay "external" either by its nature
(e.g. it's only useful to your very particular case) or for whichever
other reason.


        Stefan

Proposal for a closed-buffer tracker

[Kelly Dean]

Below is code for a closed-buffer tracker. It lets you reopen closed buffers, and restores the major mode, minor modes, point, mark, mark ring, and other buffer-local variables returned by the function desktop-buffer-info. Currently, it's implemented only for file-visiting buffers. It's comparable to the «closed tabs» feature of modern web browsers, and useful for the same reasons.

Note that although it uses functions in desktop.el, it doesn't require desktop-save-mode to be enabled; the two operate independently. Also note, it relies on a recent patch to trunk; it won't work on 24.4 or 24.5. And the «silently» macro is generated from second-level just because I happen to use the generator for other things not included here.

Stefan suggested I submit this feature as a patch. If other people might find it useful, should it go into desktop.el? Or perhaps GNU Elpa? Currently it's just part of a larger convenience package at:
http://prtime.org/emacs/usablizer.html

The «silently» macro and its generator are in another package (that Usablizer requires) at:
http://prtime.org/emacs/vimizer.html


;;; Utilities

(defmacro dlet (binders &rest body)
  "Like `let', but always bind dynamically, even if `lexical-binding' is t.
Uses the local-specialness feature of `defvar'."
  (unless (listp binders) (error "%S is not a list" binders))
  ;; Contain the local-specialness, so it doesn't infect «let»s outside dlet,
  ;; because the purpose of local-specialness is to avoid global infection.
  `(progn
     ,@(let (vardefs) ; Generate all the «defvar»s
	 (dolist (binder binders (nreverse vardefs))
	   (cond ((symbolp binder)
		  (push `(defvar ,binder) vardefs))
		 ((and (listp binder)
		       (symbolp (car binder)))
		  (push `(defvar ,(car binder)) vardefs))
		 (t (error "%S is not a symbol or list" binder)))))
     (let ,binders ,@body)))

(defmacro define-function-suppressor (wrapper victim docstring)
  "Make a macro named WRAPPER (a symbol), with DOCSTRING, that takes a body
and evaluates it with function VICTIM suppressed."
  `(defmacro ,wrapper (&rest body) ,docstring
	     `(cl-letf (((symbol-function ',',victim) (lambda (&rest _dummy) ())))
		,@body)))

(define-function-suppressor silently message
  "Do BODY without messaging anything.")

;; Copied from assq-delete-all in subr.el, but «eq» replaced by «equal»
(defun assoc-delete-all (key alist)
  "Delete from ALIST all elements whose car is `equal' to KEY.
Return the modified alist.
Elements of ALIST that are not conses are ignored."
  (while (and (consp (car alist))
              (equal (car (car alist)) key))
    (setq alist (cdr alist)))
  (let ((tail alist) tail-cdr)
    (while (setq tail-cdr (cdr tail))
      (if (and (consp (car tail-cdr))
               (equal (car (car tail-cdr)) key))
          (setcdr tail (cdr tail-cdr))
        (setq tail tail-cdr))))
  alist)


;;; Closed-buffer tracker. Inspired by:
;;; http://stackoverflow.com/questions/2227401/how-to-get-a-list-of-last-closed-files-in-emacs

(defvar closed-buffer-history nil
  "Reverse chronological list of closed buffers.
This list stores filenames and/or full buffer states as stored by
`desktop-save-mode', including point, mark, and various other buffer-local
variables.
The list size is limited by `closed-buffer-history-max-saved-items' and
`closed-buffer-history-max-full-items'.
When a buffer already in the list is closed again, it's moved to the head of
the list.")

(defvar closed-buffer-history-max-saved-items 1000
  "Max items to save on `closed-buffer-history' list.
Use -1 for unlimited, or zero to disable tracking closed files.
If disabled after having been enabled, `closed-buffer-history' will retain
the list from when it was enabled, even though no new items will be added to
it. To clear the list, set it to nil.
See also `closed-buffer-history-max-full-items'.")

(defvar closed-buffer-history-max-full-items 100
  "Max full items to save on `closed-buffer-history' list.
Use -1 for unlimited, or zero to disable tracking of full items. If this
limit is less than `closed-buffer-history-max-saved-items', then non-full
items will be stored for the difference. If this limit is greater, then
`closed-buffer-history-max-saved-items' is the controlling limit. When new
items are added to `closed-buffer-history', full items which exceed this
limit are converted to non-full items. The purpose of that is to save space.
 A full item is a buffer state, including `buffer-file-name', `point',
`mark', `mark-ring', `major-mode', minor modes, and various other
buffer-local variables as configured for `desktop-save-mode', but excluding
the buffer contents, which are stored only in the named file. A non-full
item is just a file name.")

(defun untrack-closed-buffer (name)
  ;; Could be just name, or info list; delete in either case
  (setq closed-buffer-history
        (delete name
                (assoc-delete-all name closed-buffer-history))))

(defun track-closed-buffer ()
  (when (and buffer-file-name (not (= closed-buffer-history-max-saved-items 0)))
    ;; Remove from not-head of list
    (untrack-closed-buffer buffer-file-name)
    ;; Add to head of list
    (pushnew (if (desktop-save-buffer-p buffer-file-name (buffer-name) major-mode)
                 (cdr (save-current-buffer
                        (desktop-buffer-info (current-buffer))))
               buffer-file-name)
             closed-buffer-history)
    ;; Truncate excess elements
    (let* ((max-full closed-buffer-history-max-full-items)
           (max-saved closed-buffer-history-max-saved-items)
           (truncatees (nthcdr max-saved closed-buffer-history))
           demotees)
      (and (> max-saved 0) truncatees (setcdr truncatees nil))
      (unless (< max-full 0)
        (setq demotees (nthcdr max-full closed-buffer-history))
        ;; Demote buffer info lists to filenames.
        (letrec ((demote (lambda (x) (when (and (consp x) (consp (car x)))
                                       (setcar x (caar x)) (funcall demote (cdr x))))))
          (funcall demote demotees))))))

(defun reopen-buffer (name &optional remove-missing select)
  "Open file, and restore buffer state if recorded in `closed-buffer-history'.
Return buffer for the opened file, or nil if not listed in `closed-buffer-history'.

If unable to open file, then remove from `closed-buffer-history' if confirmed
interactively or REMOVE-MISSING is non-nil, or signal error if it is
nil and reopen-buffer was not called interactively.

If called interactively, or SELECT is non-nil, then switch to the buffer."
  (interactive
   (list (ido-completing-read "Last closed: "
                              (mapcar (lambda (x) (if (consp x) (car x) x))
                                      closed-buffer-history)
                              nil t) nil t))
  (let* ((bufinfo (assoc name closed-buffer-history))
         (bufinfo (or bufinfo (if (memq name closed-buffer-history)
                                  (make-list 8 nil)))))
    (assert bufinfo)
    ;;Load from info list, using base filename as new buffer name.
    (let ((buf
           ;; Set variables needed by desktop-create-buffer.
           ;; Need dlet because they're not globally special, but only locally
           ;; special in desktop.el, which according to Stefan, is not weird.
           (dlet ((desktop-buffer-ok-count 0)
                  (desktop-buffer-fail-count 0)
                  desktop-first-buffer)
                 (silently ; Silence desktop-restore-file-buffer if file can't be found
                  (apply 'desktop-create-buffer (string-to-number desktop-file-version)
                         name (file-name-nondirectory name) (cddr bufinfo))))))
      (if buf (progn
                (untrack-closed-buffer name)
                (with-current-buffer buf (run-hooks 'desktop-delay-hook))
                (setq desktop-delay-hook nil)
                (when select
                  ;; 3 lines copied from desktop-restore-file-buffer in desktop.el
                  (condition-case nil
                      (switch-to-buffer buf)
                    (error (pop-to-buffer buf))))
                buf)
        (when (or remove-missing
                  (and
                   (called-interactively-p 'any)
                   (y-or-n-p (format
                              "Failed to open file %s; remove from closed-buffer-history? "
                              name))))
          (untrack-closed-buffer name))
        (unless (or remove-missing (called-interactively-p 'any))
          (error "Failed to open file %s" name))))))

(add-hook 'kill-buffer-hook #'track-closed-buffer)
(global-set-key [S-XF86Close] #'reopen-buffer)

Re: Proposal for a closed-buffer tracker

[Eli Zaretskii]

> From: Kelly Dean <kelly@prtime.org>
> Date: Sun, 22 Feb 2015 04:11:54 +0000
> 
> Below is code for a closed-buffer tracker. It lets you reopen closed buffers, 
> and restores the major mode, minor modes, point, mark, mark ring, and other 
> buffer-local variables returned by the function desktop-buffer-info. Currently, 
> it's implemented only for file-visiting buffers. It's comparable to the «closed 
> tabs» feature of modern web browsers, and useful for the same reasons.

Thanks.

> Note that although it uses functions in desktop.el, it doesn't require 
> desktop-save-mode to be enabled; the two operate independently.

If by "independently" you mean that desktop saving does not have to be
enabled, then fine.  But this functionality should IMO be able to do
everything desktop.el is able to do wrt restoring buffers.  In
particular, desktop.el is able to restore buffers whose
buffer-file-name is nil, at least for Info buffers.  I'd presume that
buffers visiting URLs via eww should also be restorable (I don't know
if desktop.el can do that already, but if not, it should be taught to
do so, and this feature should then be able to reuse that).

> Stefan suggested I submit this feature as a patch. If other people might find 
> it useful, should it go into desktop.el? Or perhaps GNU Elpa? Currently it's 
> just part of a larger convenience package at:
> http://prtime.org/emacs/usablizer.html

IMO, it should definitely go to desktop.el, as it uses its
infrastructure.

> (defvar closed-buffer-history-max-saved-items 1000

This should be a defcustom, of course.

>   "Max items to save on `closed-buffer-history' list.
> Use -1 for unlimited, or zero to disable tracking closed files.

I think we tend to use nil for "unlimited", not -1.

Please use 2 spaces between sentences in doc strings, we use the US
English style.

> To clear the list, set it to nil.

The list is an internal variable, so telling users to set it is a
minor annoyance, I think.  How about an explicit function to do that,
a-la "Clear Recent History"?

> (defvar closed-buffer-history-max-full-items 100
>   "Max full items to save on `closed-buffer-history' list.

Should be a defcustom.

> Use -1 for unlimited, or zero to disable tracking of full items.

Again, nil for unlimited is better, IMO.

>   (interactive
>    (list (ido-completing-read "Last closed: "

Why ido-completing-read, rather than completing-read?  I think it6's
up to the users to customize their completion.

>     ;;Load from info list, using base filename as new buffer name.
>     (let ((buf
>            ;; Set variables needed by desktop-create-buffer.
>            ;; Need dlet because they're not globally special, but only locally
>            ;; special in desktop.el, which according to Stefan, is not weird.
>            (dlet ((desktop-buffer-ok-count 0)
>                   (desktop-buffer-fail-count 0)
>                   desktop-first-buffer)
>                  (silently ; Silence desktop-restore-file-buffer if file can't be found
>                   (apply 'desktop-create-buffer (string-to-number 
> desktop-file-version)
>                          name (file-name-nondirectory name) (cddr bufinfo))))))

What happens if there is already a buffer by the same name?  Also,
should we somehow draw user's attention to the fact that another
buffer visits the same file?

> (add-hook 'kill-buffer-hook #'track-closed-buffer)

Why the hash?

> (global-set-key [S-XF86Close] #'reopen-buffer)

Why this strange binding?

Also, if we are targeting newbies who are accustomed to Web browsers,
this functionality should be easily reachable from the menu bar, IMO.

Thanks again for working on this.

Re: Proposal for a closed-buffer tracker

[Stefan Monnier]

> Below is code for a closed-buffer tracker. It lets you reopen closed
> buffers, and restores the major mode, minor modes, point, mark, mark
> ring, and other buffer-local variables returned by the function
> desktop-buffer-info. Currently, it's implemented only for
> file-visiting buffers. It's comparable to the «closed tabs» feature
> of modern web browsers, and useful for the same reasons.

[ FWIW: I'd be really happy if someone were to design some clean
  "buffer-info" system, which can then be used for desktop,
  for bookmark, for special-mode's revert-buffed, and for help-xref
  (and now for closed-buffer).  ]

Sounds nice.

> Stefan suggested I submit this feature as a patch. If other people
> might find it useful, should it go into desktop.el?

In light of my view that the "buffer-info" you currently get from
desktop-buffer-info should really not be part of desktop, you can
probably guess that I think closed-buffer should be in a package of
its own.
Of course, it should use names with a proper package prefix.

> Or perhaps GNU ELPA?

Sounds good, yes.

>   (unless (listp binders) (error "%S is not a list" binders))
[...]
> 	 (dolist (binder binders (nreverse vardefs))

The `listp' test above is not really needed since dolist should signal an
equivalent error anyway.

> 	   (cond ((symbolp binder)
> 		  (push `(defvar ,binder) vardefs))
> 		 ((and (listp binder)
> 		       (symbolp (car binder)))
> 		  (push `(defvar ,(car binder)) vardefs))
> 		 (t (error "%S is not a symbol or list" binder)))))

BTW, you can do:

	   (push `(defvar ,(cond ((symbolp binder) binder)
		                 ((and (listp binder)
		                       (symbolp (car binder)))
		                  (car binder))
		                 (t (error "%S is not a symbol or list" binder))))
	         vardefs)

And you can skip the explicit `listp' test again which `car' will do for
you as well as the `symbolp' test which `defvar' will do for you:

	   (push `(defvar ,(if (symbolp binder) binder (car binder)))
	         vardefs)

Other than that, looks very similar to what I have in cconv.el:

;; (defmacro dlet (binders &rest body)
;;   ;; Works in both lexical and non-lexical mode.
;;   (declare (indent 1) (debug let))
;;   `(progn
;;      ,@(mapcar (lambda (binder)
;;                  `(defvar ,(if (consp binder) (car binder) binder)))
;;                binders)
;;      (let ,binders ,@body)))

> (defmacro define-function-suppressor (wrapper victim docstring)

I don't much like this kind of hacking, although I know it can be
very useful.  IOW: when you need it, go right ahead, but don't forget to
report that need as a bug.


        Stefan

Re: Proposal for a closed-buffer tracker

[Stefan Monnier]

>> (add-hook 'kill-buffer-hook #'track-closed-buffer)
> Why the hash?

I consider it good practice to use #'<name> rather than '<name> when
referring to a function rather than to a symbol (tho it's not used very
widely right now).  Among the benefits, the byte-compiler will check
that this function indeed exists.


        Stefan

Re: Proposal for a closed-buffer tracker

[Dmitry Gutov]

On 02/23/2015 12:03 AM, Stefan Monnier wrote:

> I consider it good practice to use #'<name> rather than '<name> when
> referring to a function rather than to a symbol (tho it's not used very
> widely right now).

It's been gaining in popularity recently.

https://github.com/bbatsov/emacs-lisp-style-guide/commit/26a37b77af54614a41437906f8062fb1744e5889

Re: Proposal for a closed-buffer tracker

[Artur Malabarba]

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

On Feb 22, 2015 7:23 PM, "Dmitry Gutov" <dgutov@yandex.ru> wrote:
>
> On 02/23/2015 12:03 AM, Stefan Monnier wrote:
>
>> I consider it good practice to use #'<name> rather than '<name> when
>> referring to a function rather than to a symbol (tho it's not used very
>> widely right now).
>
>
> It's been gaining in popularity recently.
>
>
https://github.com/bbatsov/emacs-lisp-style-guide/commit/26a37b77af54614a41437906f8062fb1744e5889

Not to mention Emacs source should try to set an example. So IMO, yes,
please do that.

[-- Attachment #2: Type: text/html, Size: 844 bytes --]

Re: Proposal for a closed-buffer tracker

[Eli Zaretskii]

> Date: Mon, 23 Feb 2015 13:53:39 +0000
> From: Artur Malabarba <bruce.connor.am@gmail.com>
> Cc: Kelly Dean <kelly@prtime.org>, Eli Zaretskii <eliz@gnu.org>,
> 	Stefan Monnier <monnier@iro.umontreal.ca>,
> 	emacs-devel <emacs-devel@gnu.org>
> 
> On Feb 22, 2015 7:23 PM, "Dmitry Gutov" <dgutov@yandex.ru> wrote:
> >
> > On 02/23/2015 12:03 AM, Stefan Monnier wrote:
> >
> >> I consider it good practice to use #'<name> rather than '<name> when
> >> referring to a function rather than to a symbol (tho it's not used very
> >> widely right now).
> >
> >
> > It's been gaining in popularity recently.
> >
> >
> https://github.com/bbatsov/emacs-lisp-style-guide/commit/26a37b77af54614a41437906f8062fb1744e5889
> 
> Not to mention Emacs source should try to set an example. So IMO, yes, please
> do that. 

All of which are IMO questionable ways of announcing new style
conventions in Emacs development.

Re: Proposal for a closed-buffer tracker

[Artur Malabarba]

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

I'm refrain from repeating some of the things Eli or Stefan already pointed
out:

> should it go into desktop.el? Or perhaps GNU Elpa?

If it can be made into its own package, then yes, it should go on Gelpa as
well. But that doesn't mean it can't be built-in. I think this is the type
of feature that needs to be turned on by default.

> (defmacro dlet (binders &rest body)

If this remains, it needs to go somewhere else (like subr-x), or be given a
prefix.

> (defmacro define-function-suppressor (wrapper victim docstring)

I understand this comes from somewhere else, but even in a general context
this macro looks like overkill to me. Just define `silently' (and other
similar macros) as a regular macro.

> ;; Copied from assq-delete-all in subr.el, but «eq» replaced by «equal»
> (defun assoc-delete-all (key alist)
>   "Delete from ALIST all elements whose car is `equal' to KEY.
> Return the modified alist.
> Elements of ALIST that are not conses are ignored."
>   (while (and (consp (car alist))
>               (equal (car (car alist)) key))
>     (setq alist (cdr alist)))
>   (let ((tail alist) tail-cdr)
>     (while (setq tail-cdr (cdr tail))
>       (if (and (consp (car tail-cdr))
>                (equal (car (car tail-cdr)) key))
>           (setcdr tail (cdr tail-cdr))
>         (setq tail tail-cdr))))
>   alist)

I also find this a little corner-case to warrant so much code. I would just
use
    (cl-remove-if (lambda (x) (equal (car-safe x) key)) alist)
Or even
    (cl-remove key alist :filter #'car-safe)

> (defun untrack-closed-buffer (name)
>   ;; Could be just name, or info list; delete in either case

This comment might as well be a docstring.

> (defun track-closed-buffer ()
>   (when (and buffer-file-name (not (=
closed-buffer-history-max-saved-items 0)))
>     ;; Remove from not-head of list
>     (untrack-closed-buffer buffer-file-name)
>     ;; Add to head of list
>     (pushnew (if (desktop-save-buffer-p buffer-file-name (buffer-name)
major-mode)

Use cl-pushnew (also [1] for reference below).

>         (letrec ((demote (lambda (x) (when (and (consp x) (consp (car x)))
>                                        (setcar x (caar x)) (funcall
demote (cdr x))))))

This will stop demoting if it runs into a non-full entry. Is it guaranteed
that a non-full entry can only be followed by non-full entries?
Line [1] above makes it seem like non-full entries might get inserted at
the head, which means you can have non full entries in front of full
entries.

> (defun reopen-buffer (name &optional remove-missing select)
> If called interactively, or SELECT is non-nil, then switch to the buffer."

Would be nice if prefix argument did something. How about setting SELECT to
nil?

[-- Attachment #2: Type: text/html, Size: 3367 bytes --]