Can we change `col-type` checking to another place?

[daninus14]

Discussed in https://github.com/fukamachi/mito/discussions/154

^{Originally posted by daninus14 October 31, 2024} Hi,

In the code for initialize-instance here (copied below for convenience): https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/class/column.lisp#L73

(defmethod initialize-instance :around ((class table-column-class) &rest rest-initargs
                                        &key name initargs ghost
                                        &allow-other-keys)
  (declare (ignore ghost))
  (unless (find (symbol-name name) initargs :test #'string=)
    ;; Add the default initarg.
    (push (intern (symbol-name name) :keyword)
          (getf rest-initargs :initargs)))

  (let ((class (apply #'call-next-method class rest-initargs)))
    (unless (slot-boundp class 'col-type)
      (if (or (ghost-slot-p class)
              (slot-value class 'references))
          (setf (slot-value class 'col-type) nil)
          (error 'col-type-required
                 :slot class)))
    class))

A check for col-type's presence is done by this code:

(unless (slot-boundp class 'col-type)
      (if (or (ghost-slot-p class)
              (slot-value class 'references))
          (setf (slot-value class 'col-type) nil)
          (error 'col-type-required
                 :slot class)))

This is causing issues for me with compute-effective-slot-definition here https://github.com/sbcl/sbcl/blob/master/src/pcl/std-class.lisp#L1278

Here's the code for reference:

(defmethod compute-effective-slot-definition ((class slot-class) name dslotds)
  (let* ((initargs (compute-effective-slot-definition-initargs class dslotds))
         (class (apply #'effective-slot-definition-class class initargs))
         (slotd (apply #'make-instance class initargs)))
    slotd))

Because before I can setf the col-type myself in my own compute-effective-slot-definition method, it's signaling an error.

Why is the error signaled on initialize-instance? Could we change that to somewhere else like whenever generating the SQL where it may make more sense? When we generate the SQL we need col-type to be defined. However at the time of initialization, I don't think it's needed.

I think this is more sensible and allows for the project to be extensible. What do you think?

[daninus14]

From here it looks like it's used in only 7 source files: https://github.com/search?q=repo%3Afukamachi%2Fmito%20col-type&type=code

1. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/dao/mixin.lisp#L120
2. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/dao/mixin.lisp#L120
3. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/dao/table.lisp#L56
4. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/conversion.lisp#L23
5. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/class/column.lisp#L24
6. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/class/table.lisp#L48
7. https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/dao/column.lisp#L91

Every single one that could be problematic calls (table-column-type column) or some other accessor. Calling an accessor on an unbound slot will give an error anyways already. I think the benefit of the current code in https://github.com/fukamachi/mito/blob/9a2f926153aeac097498f6f3047095e77c8c1e5e/src/core/class/column.lisp#L73 is to help during development of mito, but functionality wise, the code will anyways produce an error if col-type is unbound when it shouldn't.

Therefore I think removing this error signaling should not actually affect the functionality of mito in any way, and it's safe to remove.

What do you think? Do you agree? Would you accept a PR?

[daninus14]

I added a PR here: https://github.com/fukamachi/mito/pull/155

It looks like one of the tests failed here https://github.com/fukamachi/mito/actions/runs/11612951970/job/32337587093#step:3:2828

Any ideas why?