Issues with new SetOpaque API

[bptato]

Attempting to upgrade to the new version (?), I've noticed some oddities with SetOpaque:

• Since #658 it can fail. But it returns JS_FALSE (0) on success and JS_TRUE (1) on failure, which I find somewhat counter-intuitive. Maybe it's not too late to change it?
• Previously, I could set an opaque on the global object, but now I can't because it's a JS_CLASS_OBJECT which is < JS_CLASS_INIT_COUNT.

The latter one is a bit problematic, because my type conversion functions expect the global object to have an opaque. I would change its class ID if I could, but at least when I was writing my wrapper for qjs I couldn't, and I don't see any changes in -ng that would enable this.

I don't know what the preferred fix is for this one; this patch works for me:

diff --git a/quickjs.c b/quickjs.c
index 9cac6de..e268ce3 100644
--- a/quickjs.c
+++ b/quickjs.c
@@ -10102,7 +10102,8 @@ JS_BOOL JS_SetOpaque(JSValue obj, void *opaque)
     if (JS_VALUE_GET_TAG(obj) == JS_TAG_OBJECT) {
         p = JS_VALUE_GET_OBJ(obj);
         // User code can't set the opaque of internal objects.
-        if (p->class_id >= JS_CLASS_INIT_COUNT) {
+        if (p->class_id >= JS_CLASS_INIT_COUNT ||
+            p->class_id == JS_CLASS_OBJECT) {
             p->u.opaque = opaque;
             return 0;
         }

[saghul]

Hey there!

Many APIs return 0 for success and -1 for error. I guess we could align it to that. JS_BOOL is indeed probably not the right return type here.

I took a quick look and JS_CLASS_OBJECT does not seem to use the union. That said, what is it that you are using the opaque for? If it's for hiding stuff perhaps a Symbol would do? Or the context / runtime opaque? Since there is only one global...

[bptato]

Many APIs return 0 for success and -1 for error. I guess we could align it to that. JS_BOOL is indeed probably not the right return type here.

Sounds good :)

I took a quick look and JS_CLASS_OBJECT does not seem to use the union. That said, what is it that you are using the opaque for? If it's for hiding stuff perhaps a Symbol would do? Or the context / runtime opaque? Since there is only one global...

It's for object conversion:

https://git.sr.ht/~bptato/monoucha/tree/faff9e175ae6e6d781c4fb770a49bfa30bd6f703/item/monoucha/fromjs.nim#L309

I could check if val is the global object and then override p with some pointer from the context opaque, but I'd prefer not to add special casing logic if possible. Less code = prettier code :P