Don't expose JS_GetPropertyInternal in the public API

[saghul]

No description provided.

[chqrlie]

Good move! I have a few remarks:

• the implementation for JS_GetProperty and JS_SetProperty should probably call the Internal2 version directly (albeit the compiler will inline the intermediary call anyway).
• Ben mentioned that it would make sense for these APIs to consume a refcount for the JSAtom. I am not convinced, but in any case, the Internal version should not so the calls from the interpreter loop would not need to increment the refcount before calling the internal API. The problem is this is a major API change so possibly too late to change at this point. It does feel awkward to have to increment the refcount of JS_ATOM_length in this call: JS_GetProperty(ctx, obj, JS_ATOM_length)
• For consistency, we should have JS_GetPropertyInt64 too.

[bnoordhuis]

Ben mentioned that it would make sense for these APIs to consume a refcount for the JSAtom

Not exactly. My observation was that JS_SetProperty and friends consume / take ownership of the value but not the key, i.e., you have to JS_FreeValue the key but not the value (and that requirement isn't documented anywhere either, AFAICT.)

Half-way APIs are the worst so my suggestion was to either consume both or neither, but not one or the other.

That's definitely a backwards incompatible change, though, and a subtle one, too. I believe Charlie suggested versioning the API, to make it opt-in.

[bnoordhuis]

Style nit: line up arguments

[saghul]

Pushed an update, PTAL!

• the implementation for JS_GetProperty and JS_SetProperty should probably call the Internal2 version directly (albeit the compiler will inline the intermediary call anyway).

Done.

• Ben mentioned that it would make sense for these APIs to consume a refcount for the JSAtom. I am not convinced, but in any case, the Internal version should not so the calls from the interpreter loop would not need to increment the refcount before calling the internal API. The problem is this is a major API change so possibly too late to change at this point. It does feel awkward to have to increment the refcount of JS_ATOM_length in this call: JS_GetProperty(ctx, obj, JS_ATOM_length)

Not exactly. My observation was that JS_SetProperty and friends consume / take ownership of the value but not the key, i.e., you have to JS_FreeValue the key but not the value (and that requirement isn't documented anywhere either, AFAICT.)

Half-way APIs are the worst so my suggestion was to either consume both or neither, but not one or the other.

That's definitely a backwards incompatible change, though, and a subtle one, too. I believe Charlie suggested versioning the API, to make it opt-in.

I think any change there is to be done in another PR.

That said, to Ben's point on half-way APIs: looks like there is some (weird) consistency going on: atoms are never consumed in JS APIs. Perhaps a way around it would be with some documentation macros so the caller would see if a reference would be consumed or not. I don't have good names at the tip of my tongue though.

• For consistency, we should have JS_GetPropertyInt64 too.

Makes sense! I'll add that.

[chqrlie]

• Ben mentioned that it would make sense for these APIs to consume a refcount for the JSAtom. I am not convinced, but in any case, the Internal version should not so the calls from the interpreter loop would not need to increment the refcount before calling the internal API. The problem is this is a major API change so possibly too late to change at this point. It does feel awkward to have to increment the refcount of JS_ATOM_length in this call: JS_GetProperty(ctx, obj, JS_ATOM_length)

Not exactly. My observation was that JS_SetProperty and friends consume / take ownership of the value but not the key, i.e., you have to JS_FreeValue the key but not the value (and that requirement isn't documented anywhere either, AFAICT.)
Half-way APIs are the worst so my suggestion was to either consume both or neither, but not one or the other.
That's definitely a backwards incompatible change, though, and a subtle one, too. I believe Charlie suggested versioning the API, to make it opt-in.

I think any change there is to be done in another PR.

After further thinking, I think we should just leave it as is for now.

That said, to Ben's point on half-way APIs: looks like there is some (weird) consistency going on: atoms are never consumed in JS APIs.

Yes, the atoms are never consumed. This is especially true for JS_SetProperty called from the bytecode interpreter that always passes atoms that should not be consumed. Also many calls use constant atom values such as JS_ATOM_length.

Perhaps a way around it would be with some documentation macros so the caller would see if a reference would be consumed or not. I don't have good names at the tip of my tongue though.

This was the very purpose of JSValue vs JSValueConst: a hint to the programmer (including ourselves) that a JSValue is owned by the function and should be consumed whereas a JSValueConst is not owned, should be dupped to get ownership if needed. The name was unfortunate, a side effect of a compiler trick used to try and detect refcounting errors at compile time. This validation phase did not produce a valid executable but did help finding some mistakes. The new gc with cycle detection also helped detect missing calls to JS_FreeValue.

It would be nice to have tool to verify refcounting on JSValue, JSString, JSAtom, etc statically (at compile time), using more explicit annotations.

[saghul]

Agreed. Can you please take a final look? Is this good to go?

[chqrlie]

Agreed. Can you please take a final look? Is this good to go?

The prototype for JS_GetPropertyInternal in quickjs.c seems misaligned.

[saghul]

Agreed. Can you please take a final look? Is this good to go?

The prototype for JS_GetPropertyInternal in quickjs.c seems misaligned.

Ops, I had missed that one. Fixed!