denoland/rusty_v8

Symbol::for_global incorrectly using ForApi, returns symbols from separate registry

lrowe opened this issue · 3 comments

v8 provides two methods on Symbol for returning global symbols, one of which uses a separate registry, see: https://v8.github.io/api/head/classv8_1_1Symbol.html:

For()

static Local<Symbol> v8::Symbol::For(Isolate * isolate, Local< String > description)
Access global symbol registry. Note that symbols created this way are never collected, so they should only be used for statically fixed properties. Also, there is only one global name space for the descriptions used as keys. To minimize the potential for clashes, use qualified names as keys.

ForApi()

static Local<Symbol> v8::Symbol::ForApi(Isolate * isolate, Local< String > description)
Retrieve a global symbol. Similar to |For|, but using a separate registry that is not accessible by (and cannot clash with) JavaScript code.

In rusty_v8 for_global is described as being equivalent to For(), see: https://docs.rs/v8/0.76.0/v8/struct.Symbol.html#method.for_global

for_global()

pub fn for_global<'s>(scope: &mut HandleScope<'s, ()>, description: Local<String>) -> Local<'s, Symbol>
Access global symbol registry. Note that symbols created this way are never collected, so they should only be used for statically fixed properties. Also, there is only one global description space for the descriptions used as keys. To minimize the potential for clashes, use qualified descriptions as keys. Corresponds to v8::Symbol::For() in C++.

But in the code we can see it uses v8__Symbol__ForApi:

rusty_v8/src/symbol.rs

Lines 60 to 71 in 0f06970

#[inline(always)]
pub fn for_global<'s>(
scope: &mut HandleScope<'s, ()>,
description: Local<String>,
) -> Local<'s, Symbol> {
unsafe {
scope.cast_local(|sd| {
v8__Symbol__ForApi(sd.get_isolate_ptr(), &*description)
})
}
.unwrap()
}

I imagine rusty_v8 should expose both APIs. I think it would make sense to expose for_api with the current implementation and fix for_global to match its description and call v8__Symbol__For.

A GitHub search of the denoland org does not show any current use of for_global other than the rusty_v8 tests.

Good catch. I think we should deprecate this method and add the explicit for/for_api versions.

I don't think we can use for since it is a keyword. Any ideas on what to name it?

~What about for_name? ~

for_key might be a better name, as there's a matching keyFor in the JS API.