Skip to content

Fix for SLOT-MISSING loses slot name under custom metaclasses#738

Closed
blakemcbride wants to merge 1 commit intoarmedbear:masterfrom
blakemcbride:master
Closed

Fix for SLOT-MISSING loses slot name under custom metaclasses#738
blakemcbride wants to merge 1 commit intoarmedbear:masterfrom
blakemcbride:master

Conversation

@blakemcbride
Copy link
Copy Markdown

Summary

When SLOT-VALUE (or SETF SLOT-VALUE, SLOT-BOUNDP, SLOT-MAKUNBOUND) is
called with a slot name that does not name a slot of the instance's
class, ABCL invokes SLOT-MISSING -- but if the instance's class has a
custom metaclass (any subclass of STANDARD-CLASS other than
STANDARD-CLASS / FUNCALLABLE-STANDARD-CLASS / STRUCTURE-CLASS itself),
the SLOT-NAME argument arrives at SLOT-MISSING as NIL. The original
slot name is lost before it reaches any user code.

SBCL 2.6.x and CCL 1.13.x both preserve the slot name in this case,
as required by the ANSI CL SLOT-MISSING specification. With this
bug, portable MOP code that dispatches on slot name inside
SLOT-MISSING cannot work on ABCL under a custom metaclass.

Root cause

The bug is in src/org/armedbear/lisp/clos.lisp, in SLOT-VALUE and the
three related entry points SET-SLOT-VALUE (i.e. (SETF SLOT-VALUE)),
SLOT-BOUNDP, and SLOT-MAKUNBOUND. Each of these functions has two
branches: a fast path for the standard metaclasses, and a generic
path that dispatches through SLOT-VALUE-USING-CLASS (or the BOUNDP /
MAKUNBOUND variants) for everything else.

The generic path looked like this (SLOT-VALUE shown; the others are
identical in shape):

(defun slot-value (object slot-name)
  (let* ((class (class-of object))
         (metaclass (class-of class)))
    (if (or (eq metaclass +the-standard-class+)
            (eq metaclass +the-structure-class+)
            (eq metaclass +the-funcallable-standard-class+))
        (std-slot-value object slot-name)
        (slot-value-using-class class object
                                (find-slot-definition class slot-name)))))

When the slot does not exist, FIND-SLOT-DEFINITION returns NIL. That
NIL is then passed to SLOT-VALUE-USING-CLASS as the third argument.
NIL is a symbol, so dispatch selects the

(defmethod slot-value-using-class ((class standard-class)
                                   instance
                                   (slot symbol))
  (std-slot-value instance slot))

method, which calls (STD-SLOT-VALUE INSTANCE NIL). At that point the
original slot name is already gone: STD-SLOT-VALUE looks up NIL in
the layout, fails to find it, and bottoms out in SLOT-MISSING with
SLOT-NAME bound to NIL.

The fast path is unaffected because STD-SLOT-VALUE on the object's
own class sees the real slot-name and forwards it to SLOT-MISSING
correctly.

Fix

The generic path must detect the missing-slot case itself and invoke
SLOT-MISSING directly with the original slot-name, instead of feeding
NIL into SLOT-VALUE-USING-CLASS. This is exactly what SBCL does
(pcl/slots-boot.lisp, the SLOT-VALUE stub):

(if (not slotd)
    (values (slot-missing class object slot-name 'slot-value))
    (slot-value-using-class class object slotd))

The patch below applies the same shape to all four entry points in
clos.lisp. Per the ANSI spec for SLOT-MISSING:

  • SLOT-VALUE uses only the primary value of SLOT-MISSING.
    Wrapping the call in VALUES enforces that contract.

  • (SETF SLOT-VALUE) ignores SLOT-MISSING's values and returns
    NEW-VALUE.

  • SLOT-BOUNDP returns a boolean derived from SLOT-MISSING's
    primary value.

  • SLOT-MAKUNBOUND ignores SLOT-MISSING's values and returns the
    instance.

No change to StandardObject.java is needed: the Java-side
STD-SLOT-VALUE / SET-STD-SLOT-VALUE / SLOT-BOUNDP primitives already
forward the slot-name to SLOT-MISSING correctly; they were simply
never being reached with the true slot-name on the custom-metaclass
path.

Verification

Before the patch (ABCL 1.9.3-dev-svn-15795M):

slot-missing got slot-name = NONESUCH (instance WITH-STD-META)
slot-missing got slot-name = NIL      (instance WITH-CUSTOM-META)

After the patch (ABCL 1.9.3-dev-svn-15823M, rebuilt from the modified
source), running the reproducer from the original bug report plus
parallel tests for (SETF SLOT-VALUE), SLOT-BOUNDP, and SLOT-MAKUNBOUND:

slot-missing got slot-name = NONESUCH (instance WITH-STD-META)
slot-missing got slot-name = NONESUCH (instance WITH-CUSTOM-META)
slot-missing got slot-name = NONESUCH (instance WITH-STD-META)
slot-missing got slot-name = NONESUCH (instance WITH-CUSTOM-META)
slot-missing got slot-name = NONESUCH (instance WITH-STD-META)
slot-missing got slot-name = NONESUCH (instance WITH-CUSTOM-META)
slot-missing got slot-name = NONESUCH (instance WITH-STD-META)
slot-missing got slot-name = NONESUCH (instance WITH-CUSTOM-META)

This matches the output produced by SBCL and CCL on the same program.

Notes

  1. The (SLOT SYMBOL) methods on SLOT-VALUE-USING-CLASS etc. remain in
    place. They are still reachable when user code calls
    SLOT-VALUE-USING-CLASS directly with a symbol, which is legal under
    ABCL's current MOP surface even though AMOP specifies the third
    argument as an effective-slot-definition. This patch only stops
    SLOT-VALUE from passing NIL into them.

  2. No existing ANSI or MOP tests should change behaviour: when
    FIND-SLOT-DEFINITION returns a real slotd, the code path is
    identical to before. Only the previously-broken missing-slot
    branch changes.

  3. CHANGES entry suggestion:

    • Fixed SLOT-MISSING receiving NIL as the slot name when
      SLOT-VALUE, (SETF SLOT-VALUE), SLOT-BOUNDP, or SLOT-MAKUNBOUND
      was called on an instance whose class uses a custom metaclass.
      The original slot name is now preserved, matching SBCL and CCL.

@blakemcbride
Copy link
Copy Markdown
Author

I am going to re-create this pull request correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant