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
:
Lines 60 to 71 in 0f06970
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.