Bugs in two functions using load-history.

[Luc Teirlinck]

I am using emacs21.2.90.

I found bugs in two functions resulting from an inappropriate use of
load-history.  The two bugs are unrelated but have exactly the same
cause.  One is very nasty.  The two functions are symbol-file and
unload-feature.

The basic problem that underlies the two bugs is that load-history
records which file defined which symbols, without any record of
whether they were defined as variables or as functions.  This latter
information would appear to be essential for most correct uses of
load-history.

To see both bugs do the following.

Start a new emacs-q.

Do M-: (symbol-file '*)

Result: nil. (Of course: built-in function)

M-: (load "ielm")

M-: (symbol-file '*)

Result: "ielm"

Ielm defines a variable *, not the function *.  Symbol-file is
supposed to return the file defining the function *.  (Otherwise,
describe-function should not use it.)  Hence this is a bug.

This bug as implications at the user level.
For instance, after loading ielm.elc, C-h f * claims that the function
* is defined in ielm:

* is a built-in function in `ielm'.
(* &rest NUMBERS-OR-MARKERS)

Returns product of any number of arguments, which are numbers or
markers.


In emacs21.2.90, things get worse from here.  Let us unload ielm:

M-: (unload-feature 'ielm)

M-: (* 2 3)

void-function error.

M-: (fboundp '*)

Result: nil.

I explicitly referred to Emacs21.2.90, because in Emacs20.7, we not
only get the expected answers, but, moreover:

M-: (symbol-file '*)

now again returns nil.

Nevertheless, unload-feature has a bug in Emacs20.7 too, which is
probably why the code got changed in Emacs21.  However, the fix made
things worse.

I know exactly what the cause for the bugs in symbol-file and
unload-feature is, but I am less sure about the best solution.

One could redefine load-history so that it distinguishes between
variable and function definitions.

Alternatively, one could make the functions that use load-history,
such as symbol-file and unload-feature (or, alternatively, the
function feature-symbols called by unload-feature) check whether any
symbol-file association they found in load-history was due to a
variable definition, a function definition or both.

Here is the bug in the Emacs21.2.90 version of unload-feature.  The
bug in the smaller function symbol-file has a similar cause:


(mapc
     (lambda (x)
       (cond ((stringp x) nil)
             ((consp x)
              ;; Remove any feature names that this file provided.
              (if (eq (car x) 'provide)
                  (setq features (delq (cdr x) features))))
             (t
              (when (boundp x)
                (makunbound x))
              (when (fboundp x)
                (if (fboundp 'ad-unadvise)
                    (ad-unadvise x))
                (fmakunbound x)
                (let ((aload (get x 'autoload)))
                  (if aload (fset x (cons 'autoload aload))))))))
     (cdr flist))

This is a piece of code from unload-feature (lines 161-177 in
loadhist.c in emacs21.2.90).

Note that both makunbound and fmakunbound get called on any symbol x
found in the file by load-history, as long as it satisfies boundp or
fboundp, respectively.  This is why the function * becomes undefined
after (unload-feature 'ielm)

Here is the bug in the Emacs20.7 version. The fact that it handles *
correctly is a coincidence:

(mapcar
     (lambda (x)
       (cond ((stringp x) nil)
             ((consp x)
              ;; Remove any feature names that this file provided.
              (if (eq (car x) 'provide)
                  (setq features (delq (cdr x) features))))
             ((boundp x) (makunbound x))
             ((fboundp x)
              (fmakunbound x)
              (let ((aload (get x 'autoload)))
                (if aload (fset x (cons 'autoload aload)))))))
     (cdr flist))

This has a differently structured cond.  It does not produce bugs if
the file defined x as a variable, but not as a function, which is
exactly what happens in the *-ielm example, but it does produce bugs
in several other situations.  There is absolutely no way whatsoever
that unload-feature can handle all situations correctly without
checking whether the file itself defined x as a variable, a function
or both.

Sincerely,

Luc.