Switch from class-star to nclasses. #2784

Ambrevar · 2023-02-10T12:30:28Z

Description

Switches from class-star to https://github.com/atlas-engineer/nclasses.

News:

No more need for the accessor transformer.
Which saves some 750 lines of code! O.o
Fixes a bunch of things.
- In particular: when the slot whas inherited, the accessor was created in the wrong package.

Fixes #1102.

Discussion

For inherited slots with no accessor, we now need to specify :accessor nil. See prompter:active-attributes-keys for instance. It makes sense, but it's a bit unwieldy. Can we do better?

To do:

Package nclasses for Guix.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

Ambrevar · 2023-02-10T15:08:22Z

I like the result, but curious what you think, @aartaka @aadcg

aartaka · 2023-02-13T10:40:41Z

I'll probably review after type inference PR is merged, so that we don't do it twice へ(゜∇、°)へ

Ambrevar · 2023-02-13T11:24:08Z

Type inference should be merged in nclasses (see atlas-engineer/nclasses#5), so it won't disturb this PR which can be review now I believe.

aartaka

All good, except that :accessor nil spilt everywhere (and not spilt somewhere it probably should be consistently spilt, like in prompter:constructor). Maybe inherit accessors from superclasses, if present? Otherwise it feels like yet another syntactical trap.

libraries/theme/theme.lisp

aartaka · 2023-02-13T15:27:20Z

source/buffer.lisp

@@ -1302,7 +1290,8 @@ proceeding."
   (prompter:enable-marks-p t)
   (prompter:actions-on-return (list (lambda-unmapped-command set-current-buffer)
                                     (lambda-mapped-command buffer-delete)
-                                     'reload-buffers))
+                                     'reload-buffers)
+                               :accessor nil)


That is weird. Should we maybe not generate accessor if it's present already? We pull moptilities in already, so why not use it for accessor inspection too?

source/command-commands.lisp

aartaka · 2023-02-13T15:34:27Z

source/command.lisp

@@ -253,8 +252,7 @@ It's the complement of `nyxt-packages' and `nyxt-user-packages'."
  ((name nil
         :type (or symbol null))
   (class-sym nil
-              :type (or symbol null)))
-  (:accessor-name-transformer (class*:make-name-transformer name)))
+              :type (or symbol null))))


But then, nil is a symbol too. However hacky it is :P

aartaka · 2023-02-13T15:35:50Z

source/lisp-system.lisp

@@ -27,7 +27,8 @@
  ((prompter:name "Quicklisp systems")
   (prompter:constructor (mapcar #'ql-dist:short-description (ql:system-list)))
   (prompter:actions-on-return (lambda-command quickload* (systems)
-                                 (ql:quickload (first systems))))))
+                                 (ql:quickload (first systems)))
+                               :accessor nil)))


That's alarming yet again...

Ambrevar · 2023-02-13T16:10:50Z

Accessors cannot easily be inherited, because unlike the situation with types, generating accessors has a global side effect. Generating the wrong accessor would be an important bug.

The issue is the same with hu.dwim.defclass-star. If you think about it, it's normal to say "do not generate an accessor for inherited slots", because the global class option is otherwise to generate accessors for all slots.

There is no way to infer that the user does not effectively want to generate the accessor from the inherited slot, as "package polluting" as it might be.

aartaka · 2023-02-13T16:13:01Z

Accessors cannot easily be inherited, because unlike the situation with types, generating accessors has a global side effect. Generating the wrong accessor would be an important bug.

The issue is the same with hu.dwim.defclass-star. If you think about it, it's normal to say "do not generate an accessor for inherited slots", because the global class option is otherwise to generate accessors for all slots.

There is no way to infer that the user does not effectively want to generate the accessor from the inherited slot, as "package polluting" as it might be.

But then, if the package is different, then the slot is most likely inherited, right? For those, we most probably, should not generate anything. It's a hacky heyristic, but a reasonable one, isn't it?

aartaka · 2023-02-17T11:59:54Z

@Ambrevar, CI was failing because closer-mop:class-finalized-p was called on NIL in nclasses. Should be fixed in atlas-engineer/nclasses@f718b16. Mind updating?

Ambrevar · 2023-02-21T09:36:09Z

Done, thanks for catching the bug!

aartaka · 2023-02-21T11:46:46Z

Oh, sems like some accessors are not generated, failing the CI...

Ambrevar · 2023-03-02T11:22:04Z

I've investigated this and there are multiple issues.

dcfd35c is logical, but I don't understand how it used to pass previously.
5c2a395 is an unfortunate workaround. Still much better than 8ee001d.

Issue 2 is a fundamental problem with CL and CLOS. Let me describe it:

parent package exports foo.
In the child package that :uses parent, there is a class with a foo slot. (find-symbol "FOO") proves that this slot name is inherited from package.
Attempting to define the accessor will define it in parent. Thus, polluting the parent package. Or outright breaking compilation if the parent package is locked.

Recipe:

(in-package :cl)
(export 'foofoo)
(defpackage :child (:use :cl))
(in-package :child)
(defclass dummy () ((foofoo :accessor foofoo)))
; => ERROR!

Any idea how we are supposed to deal with this kind of issues in CL?

aartaka · 2023-03-02T12:20:16Z

Issue 2 is a fundamental problem with CL and CLOS. Let me describe it:

parent package exports foo.

In the child package that :uses parent, there is a class with a foo slot. (find-symbol "FOO") proves that this slot name is inherited from package.

Attempting to define the accessor will define it in parent. Thus, polluting the parent package. Or outright breaking compilation if the parent package is locked.

Recipe:
(in-package :cl)
(export 'foofoo)
(defpackage :child (:use :cl))
(in-package :child)
(defclass dummy () ((foofoo :accessor foofoo)))
; => ERROR!
Any idea how we are supposed to deal with this kind of issues in CL?

The only idea that comes to my mind is checking the status of the slot symbol for being inherited. But that's yet another heuristic. Not sure that we can do any better, though (´･_･`)

Ambrevar · 2023-03-03T08:55:04Z

What do you mean? Can you give an example?

aartaka · 2023-03-03T10:07:49Z

What do you mean? Can you give an example?

I mean using find-symbol in nclasses (defclass-star.lisp, L280):

(and *automatic-accessors-p*
     (or (and (eq (symbol-package name) *package*)
              ;; NEW CHECK:
              (not (eq :inherited
                       (nth-value 1 (find-symbol name *package*)))))
         (not (eq *accessor-name-package* :slot-name))))

aartaka · 2023-03-03T11:20:31Z

Note that the snippet above has a type error, so don't use it verbatim :P

But the idea should be clear, I hope (￣(エ)￣)ゞ

Ambrevar · 2023-03-03T18:43:02Z

That changes nothing because if the symbol is inherited, then the symbol-package is the parent package, thus the new check is a no-op.

aartaka · 2023-03-06T14:01:24Z

Right...

Then, I guess, we have to live with it, debug it, and lock as many packages as we can (҂⌣̀_⌣́)

Ambrevar · 2023-03-06T17:55:47Z

debug it, and lock as many packages as we can

Debug what? And we are already locking all our packages, what do you mean?

Ambrevar · 2023-03-06T17:57:11Z

@aadcg Any insights on this?

I have one: in the case of background-buffer in nyxt/web-extensions, we can simply shadow the symbol inherited from nyxt, so that the accessor is defined in nyxt/web-extensions.

Thoughts?

aadcg · 2023-03-07T07:25:05Z

I didn't follow this PR closely as I don't think I can provide much insight... I trust your judgement!

Ambrevar · 2023-03-15T11:00:00Z

It finally passes! I'll rebase and squash.
Also need to release a new nclasses.

Ambrevar · 2023-03-15T11:39:14Z

Another issue, although this time it's an easy one: nclasses does not bind slots by default. Shall we explicitly pass nil, or shall we add an option to nclasses to default to NIL?

aadcg · 2023-03-15T12:29:05Z

Shall we explicitly pass nil, or shall we add an option to nclasses to default to NIL?

I can't think of a use case of such an option, so I'd go with the first idea.

Ambrevar · 2023-03-15T13:06:16Z

I can't think of a use case of such an option, so I'd go with the first idea.

It's standard CLOS: think of it like a symbol that can be boundp or fboundp, sometimes you want to know if it is.

I agree that it is very rarely needed in practice. One use case I can think of is when writing generic code to instantiate object, and you need to tell apart NIL from unbound.

But in this case, nclasses :unbound can be more useful...

Conclusion: I'd go with first option too!

aadcg · 2023-03-15T13:09:25Z

@Ambrevar, thanks for the clarification! It helped me to understand the context of the question :)

aartaka · 2023-03-15T13:20:30Z

Let's leave unbound that which shall be unbound.

Ambrevar · 2023-03-15T14:41:52Z

Let's leave unbound that which shall be unbound.

What do you mean? Example?

Ambrevar · 2023-03-15T16:34:56Z

The sbcl-nclasses Guix package just got updated.

Everything is ready as far as I'm concerned.

I'll merge tomorrow if there is no objection.

aartaka · 2023-03-15T20:40:36Z

Let's leave unbound that which shall be unbound.

What do you mean? Example?

I mean we shouldn't default to any value if it's not provided. Yes, :unbound may be used for explicitness, but no value should also mean unbound slot.

Rationale: unbound slot is semantically different from the NIL slot. There are slot-unbould methods that are extremely helpful for cases when slot is intended to be (initially?) unbound. There are lots of libraries (including some of mine) that have ubound slots as the actual part of the API and as something they depend on. So I suggest to leave this useful default as it is, and not infer any default value unless there actually is one.

Think of it as the (values) return: some functions actually need to return no thing. Much like for slots: some have to have no value at all, not even the false/null value. Much like we always return a list from prompter APIs: to delineate cases of no values from the cases of a single NIL value (although I still think we could've utilized multiple-value returns in there). We have to have the distinction between bound and unbound slots.

aartaka · 2023-03-15T20:43:52Z

A particular example: I use unbound slots in my cl-telegram-bot-auto-api, because Telegram APIs always return incomplete objects, in which defaulting to NIL means misinterpreting the values into being false, while they are just unbound/unknown. And that's the basis of all this library—leaving unbound that which is truly unbound/unknown.

Ambrevar · 2023-03-16T08:21:01Z

In practice, I find that NIL is a much more frequent default than :unbound. It's very clear in Nyxt.

Also adopting this default makes for minimal changes in Nyxt itself (this PR is almost exclusively limitied to removing the accessor names).

If you think this is really unreasonable, we could add a class option to specify the slot default.

aartaka · 2023-03-16T10:13:41Z

In practice, I find that NIL is a much more frequent default than :unbound. It's very clear in Nyxt.

Yes, but nclasses are also intended as a general audience library and we should be careful to not shoot off any feet :)

If you think this is really unreasonable, we could add a class option to specify the slot default.

That sounds like the best course of action, yes.

Ambrevar · 2023-03-16T13:15:35Z

OK to merge this and add the nclasses option later?

aadcg · 2023-03-16T13:25:10Z

All good from my side!

jmercouris · 2023-03-16T14:58:13Z

yes!

…ccessors.

…tion.

…asses.

Ambrevar · 2023-03-17T08:55:45Z

Done, thank you all!

aartaka · 2023-03-17T09:46:35Z

Thanks for working on it <3

Ambrevar force-pushed the switch-to-nclasses branch 2 times, most recently from 8fbf480 to fee7d5c Compare February 10, 2023 14:56

Ambrevar mentioned this pull request Feb 13, 2023

libraries/class-star: Infer type from parent slot if any. #2764

Closed

23 tasks

aartaka reviewed Feb 13, 2023

View reviewed changes

Ambrevar mentioned this pull request Feb 15, 2023

Have default-accessor-name-transformer obey :accessor-name-package. atlas-engineer/nclasses#6

Merged

Ambrevar force-pushed the switch-to-nclasses branch 2 times, most recently from 19c7c65 to 8006b4c Compare March 15, 2023 15:22

aartaka approved these changes Mar 16, 2023

View reviewed changes

Ambrevar added 10 commits March 17, 2023 09:54

Switch from class-star to nclasses.

428b714

gitmodules: Add nclasses.

ee6d8a3

libraries/prompter: Fix nclasses predicate name.

95b0f64

Configure define-class with `:accessor-name-package :slot-name'.

717e904

Fix some files:profile accesses.

cef4b1f

libraries/prompter/tests: Fix class-star -> nclasses renaming.

61d8ddc

libraries/prompter: Fix active-attributes-keys lambda list to match a…

74fe3a4

…ccessors.

web-extension: Shadow some inherited symbols to allow accessor genera…

55b13ff

…tion.

command-commands: Remove `:type t' workaround since it's fixed in ncl…

0516474

…asses.

build-scripts/nyxt.scm: Add cl-nclasses.

b266dfd

Ambrevar force-pushed the switch-to-nclasses branch from a2048bb to b266dfd Compare March 17, 2023 08:54

Ambrevar merged commit 2f39b49 into master Mar 17, 2023

Ambrevar deleted the switch-to-nclasses branch March 17, 2023 08:55

Switch from class-star to nclasses. #2784

Switch from class-star to nclasses. #2784

Conversation

Ambrevar commented Feb 10, 2023 • edited Loading

Description

Discussion

To do:

Checklist:

Ambrevar commented Feb 10, 2023

aartaka commented Feb 13, 2023

Ambrevar commented Feb 13, 2023

aartaka left a comment

Choose a reason for hiding this comment

aartaka Feb 13, 2023

Choose a reason for hiding this comment

aartaka Feb 13, 2023

Choose a reason for hiding this comment

aartaka Feb 13, 2023

Choose a reason for hiding this comment

Ambrevar commented Feb 13, 2023

aartaka commented Feb 13, 2023

aartaka commented Feb 17, 2023

Ambrevar commented Feb 21, 2023

aartaka commented Feb 21, 2023

Ambrevar commented Mar 2, 2023

aartaka commented Mar 2, 2023

Ambrevar commented Mar 3, 2023

aartaka commented Mar 3, 2023

aartaka commented Mar 3, 2023

Ambrevar commented Mar 3, 2023

aartaka commented Mar 6, 2023

Ambrevar commented Mar 6, 2023

Ambrevar commented Mar 6, 2023

aadcg commented Mar 7, 2023

Ambrevar commented Mar 15, 2023

Ambrevar commented Mar 15, 2023

aadcg commented Mar 15, 2023

Ambrevar commented Mar 15, 2023

aadcg commented Mar 15, 2023

aartaka commented Mar 15, 2023

Ambrevar commented Mar 15, 2023

Ambrevar commented Mar 15, 2023

aartaka commented Mar 15, 2023

aartaka commented Mar 15, 2023

Ambrevar commented Mar 16, 2023

aartaka commented Mar 16, 2023

Ambrevar commented Mar 16, 2023

aadcg commented Mar 16, 2023

jmercouris commented Mar 16, 2023

Ambrevar commented Mar 17, 2023

aartaka commented Mar 17, 2023

Ambrevar commented Feb 10, 2023 •

edited

Loading