COMPUTE-SLOTS incorrect for structure-class and condition-class

Bug #1049423 reported by Douglas Katzman
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
SBCL
Fix Released
Undecided
Unassigned

Bug Description

For structures, a duplicate effective-slot-definition will appear if a slot is mentioned in the slot-description list that may accompany the :INCLUDE option such as to change the default or further specify the type. But even if nothing is changed, it still manifests as a duplicate slot definition. Example:

* (defstruct animal name size)
ANIMAL
* (defstruct (dog (:include animal (name "Spot") (size))) tail-wag-arc-radians)
DOG
* (sb-pcl:class-slots (find-class 'dog))
(#<SB-PCL::STRUCTURE-EFFECTIVE-SLOT-DEFINITION NAME>
 #<SB-PCL::STRUCTURE-EFFECTIVE-SLOT-DEFINITION SIZE>
 #<SB-PCL::STRUCTURE-EFFECTIVE-SLOT-DEFINITION NAME>
 #<SB-PCL::STRUCTURE-EFFECTIVE-SLOT-DEFINITION SIZE>
 #<SB-PCL::STRUCTURE-EFFECTIVE-SLOT-DEFINITION TAIL-WAG-ARC-RADIANS>)

This affects ~70 structure classes in a pristine image, plus stuff we define using the same approach of overriding a default. It's surprising this escaped attention thus far considering that DESCRIBE is pretty obviously screwy as a consequence.

Here is a probably-working patch for the STRUCTURE-CLASS method:

diff --git a/src/pcl/std-class.lisp b/src/pcl/std-class.lisp
index 9c077f9..ffe7b65 100644
--- a/src/pcl/std-class.lisp
+++ b/src/pcl/std-class.lisp
@@ -1125,14 +1125,22 @@
     (std-compute-slots-around class eslotds)))

 (defmethod compute-slots ((class structure-class))
- (mapcan (lambda (superclass)
- (mapcar (lambda (dslotd)
- (compute-effective-slot-definition
- class
- (slot-definition-name dslotd)
- (list dslotd)))
- (class-direct-slots superclass)))
- (reverse (slot-value class '%class-precedence-list))))
+ (let (eslotds)
+ (mapc (lambda (superclass)
+ (mapc (lambda (dslotd)
+ (let* ((slot-name (slot-definition-name dslotd))
+ (eslotd
+ (compute-effective-slot-definition
+ class slot-name (list dslotd)))
+ (found (member slot-name eslotds
+ :key #'slot-definition-name
+ :test #'string=)))
+ (if found
+ (rplaca found eslotd)
+ (push eslotd eslotds))))
+ (class-direct-slots superclass)))
+ (reverse (slot-value class '%class-precedence-list)))
+ (nreverse eslotds)))

There is something unsettling about the 'string=' and I think it's wrong, but at the time I fixed this I was also looking into what I think is another bug. Namely, if you include a structure and specify a list of slots whose descriptions to alter, it doesn't "work" to refer to the slot using a symbol in the wrong package. So are they compared by print-name or aren't they? Now that I think about it more, this could be a consequence of a bug which subsumes it, namely: if you specify a slot that doesn't exist at all in the ancestor, no complaint is generated. It looks like it's just ignored.

Back to the original problem - For condition-class, the confusion is greater since multiple inheritance yields possibly N instances of a same-named direct-slot-definition that have to be merged, in the manner of standard-class. The COMPUTE-SLOTS method should probably be a hybrid of the one for STANDARD-CLASS and STRUCTURE-CLASS, if it can't just do the STANDARD-CLASS thing entirely.

./src/runtime/sbcl --version
SBCL 1.0.58.28-60bb508-dirty

Revision history for this message
Douglas Katzman (dougk) wrote :

fixed in 91f7d85a9528eb81d8da70a2900d868ee29b36b7
and then again in 1891fa69bddd2cab9180b3fa9c7fcb3206174790

Changed in sbcl:
status: New → Fix Committed
Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.