[Openmcl-devel] bad defclass causing side effect

[Gary Byers]

On Sun, 13 Jun 2004, Hamilton Link wrote:

> To me this isn't doesn't look so much like odd persistent behavior as
> undesirable side effects when you redefine a class badly (still, looks
> like a bug).
>
> The first bad defclas doesn't create a class, but once BAR is created
> (with your second call) the error from the subsequent redefinition
> should probably (but doesn't) prevent a side effect on the class. Then
> when you try to redefine the class again in a valid way, it looks at
> the now-broken class and chokes... seems like a bug just from a DWIM
> standpoint but I don't know what the letter of the law is on when
> defclass side effects take place, that would be a question for Gary.
>
> h
>

If I'd ever gotten around to responding to this, I would have dismissed
it as a not-very-interesting case of something that can happen when you
try to reinitialize in improperly/incompletely initialized class:

> On Jun 10, 2004, at 11:57 AM, Gary King wrote:
>
> > OpenMCL (0.14.2) has an odd persistent behavior with respect to class
> > definitions. If you evaluate the following four forms, you'll see what
> > I mean. The fourth form should work but somehow the error remains.
> >
> > ;;; gets an error
> > (defclass bar ()
> >   ((documentation :reader documentation)))

At this point, nothing's gotten to the point of doing a
(SETF (FIND-CLASS 'BAR) ...); a class was created and partly initialized,
but it's not (easily) globally accessible and doesn't have anything to
do with what happens next.

> >
> > ;;; works
> > (defclass bar ()
> >   ())

A class was created and is globally named.  Subsequent redefinitions
affect this class.

> >
> > ;;; gets an error
> > (defclass bar ()
> >   ((documentation :reader documentation)))

The class was modified.  It defines an accessor method, but that accessor
method's lambda list isn't congruent with the lambda list of the GF
CL:DOCUMENTATION, so ADD-METHOD fails.


> >
> > ;;; also gets an error !!
> > (defclass bar ()
> >   ())
> >

The class tries to remove its old accessor methods (which were never
added) and this fails.

I think that many similar cases of reinitializing something that
wasn't completely/correctly initialized could fail in similar ways,
and I think that DWIM is a slippery slope and that things generally
shouldn't bend over backwards too far to make things like this work:
there's a point where it's far easier for the user to say

? (setf (find-class 'bar) nil)

and start over ...

That said, the particular reason that this is failing is:


(defun remove-accessor-methods (class dslotds)
  (dolist (dslotd dslotds)
    (dolist (reader (%slot-definition-readers dslotd))
      (remove-reader-method class (ensure-generic-function reader :lambda-list '(x))))
    (dolist (writer (%slot-definition-writers dslotd))
      (remove-writer-method class (ensure-generic-function writer :lambda-list '(x y))))))

and especially the calls to ENSURE-GENERIC-FUNCTION with explicit
:LAMBDA-LIST args.  There's no particular reason to [re]initialize the
GF in this case - Gary King's example is dying trying to change the
lambda list of #'DOCUMENTATION - and (since all we really care about is
that the method is removed from the GF if the method had been added)
it's probably reasonable to stop bending forwards and bend backwards
a little:


(defun remove-accessor-methods (class dslotds)
  (dolist (dslotd dslotds)
    (dolist (reader (%slot-definition-readers dslotd))
      (let* ((f (fboundp reader))) ; FBOUNDP will return the same
                                   ; thing that SYMBOL-FUNCTION would
                                   ; (if applicable) in OpenMCL
        (when (typep f 'generic-function))
          (remove-reader-method class f)))
    (dolist (writer (%slot-definition-writers dslotd))
      (let* ((f (fboundp writer)))
        (when (typep f 'generic-function))
          (remove-writer-method class f)))))

Off the top of my head, that -looks- right, and off the top of my head,
I can't think of any reason for the ENSURE-GENERIC-FUNCTION calls to have
existed (and can think of other ways in which they'd mess things up.)

REMOVE-ACCESSOR-METHODS is called from a :BEFORE method on
REINITIALIZE-INSTANCE for class objects.  The best that we can really
do at that point is to ensure that any methods defined on any of the
old class definition's slots are removed from any generic functions
that're the current definitions of the accessor methods' names.
(REMOVE-READER-METHOD and REMOVE-WRITER-METHOD do nothing if there's
no specialized method on the GF, but they do expect a GF.)

I am (again) leery of too much DWIM in cases like this (at least partly
because no two people ever seem to mean the same thing ...), but the
explicit ENSURE-GENERIC-FUNCTION :LAMBDA-LIST calls look pretty  much
like DWIDM at the moment.