Document ownership rules of C API

[bnoordhuis]

Example: it's not really documented anywhere that JS_SetProperty(ctx, obj, key, val) takes ownership of val but not key. One must call JS_FreeAtom(ctx, key) afterwards but not JS_FreeValue(ctx, val).

Another example of an undocumented constraint: you should not JS_FreeValue the elements of the argv array to your C callback. But you should JS_FreeValue the return value of JS_GetProperty(ctx, obj, key). Here too, one must JS_FreeAtom(ctx, key).

^{Neither is it documented that the first length elements of argv are valid (set to undefined) when JS calls a C function with too few arguments. Or that argc can be greater than length - which makes perfect sense but is undocumented.}

Should probably go into docs/docs/developer-guide/api.md?

[saghul]

Yes, I think the API docs are the right place. Taking a page out of (C)Python's book: https://docs.python.org/3/c-api/list.html#c.PyList_SetItem :

There is nuance in what happens to the existing property if you override it for example, and I think we cannot "document" that with a macro, so docs is better IMHO.

[bptato]

(I'm probably missing something obvious here, but...) why not just use JSValueConst?
At least I found it convenient to see ownership information directly in function signatures, if only because it let me "jump to definition" to check if I was using the API right.

There is nuance in what happens to the existing property if you override it for example, and I think we cannot "document" that with a macro, so docs is better IMHO.

My understanding of JSValueConst is

• if a function takes JSValue, I lose the reference (so I must dup the JSValue first if I want to keep using it)
• if a function takes JSValueConst, I keep the reference
• if I get a JSValue, it comes with a reference which I must give away/free at some point
• if I get a JSValueConst (in a callback), I must dup it to get an owned reference
• (I think APIs consuming JSAtom never take a reference, and APIs returning JSAtom always give it away.)

So when overriding an existing property, the reference to the old value is already owned by the engine - I don't have to care about what happens to it as long as I follow the above rules.

(I guess documenting that "the old property gets unref'd" would still be useful for users unfamiliar with the API's design, but that doesn't decrease the value of JSValueConst, which had simple rules that in theory could be mechanically interpreted.)

[bnoordhuis]

if a function takes JSValueConst, I keep the reference

I don't think it worked like that, not 100% of the time anyway. For example, the old definition of JSCFunctionData was this:

typedef JSValue JSCFunctionData(JSContext *ctx, JSValueConst this_val, int argc, JSValueConst *argv, int magic, JSValue *func_data)

But you don't get to keep the reference to func_data, it's managed by quickjs. Oversight maybe, but there were more places where inconsistencies showed up.

I think APIs consuming JSAtom never take a reference, and APIs returning JSAtom always give it away

That's right.

[..] JSValueConst, which had simple rules that in theory could be mechanically interpreted

Yes, it was originally intended for CONFIG_CHECK_JSVALUE builds, see commit 4d57997, although I don't remember a time where it actually worked. It certainly was broken when @saghul and I started working on quickjs in earnest.

I wouldn't mind bringing back JSValueConst if we could resolve the outstanding issues. Requires that someone goes over the APIs with a fine tooth comb and revive the CONFIG_CHECK_JSVALUE build mode.

[bnoordhuis]

After going through the old code, I'm > 50% hopeful the inconsistencies are not insurmountable. Let me give it a try.

[bptato]

Ah, glad to hear that. Thanks for looking into it.

I've just tried to switch func_data to JSValueConst in bellard/quickjs; it adds a few casts and removes some, and there are weird cases like js_proxy_revoke which deletes a ref from its func_data array. But overall it just seems like an oversight.

(A drawback of the const hack that I learned from this: you have to cast arrays a lot...)