Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
lrowe opened this issue Sep 16, 2023 · 3 comments · Fixed by #1324
Closed

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

lrowe opened this issue Sep 16, 2023 · 3 comments · Fixed by #1324

Comments

@lrowe
Copy link
Contributor

lrowe commented Sep 16, 2023

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.

@mmastrac
Copy link
Contributor

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

@lrowe
Copy link
Contributor Author

lrowe commented Sep 16, 2023

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

@mmastrac
Copy link
Contributor

mmastrac commented Sep 16, 2023

~What about for_name? ~

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

lrowe added a commit to lrowe/rusty_v8 that referenced this issue Sep 17, 2023
`for_global` was documented as `for_key` but implemented as `for_api`.

Closes denoland#1323
lrowe added a commit to lrowe/rusty_v8 that referenced this issue Sep 17, 2023
`for_global` was documented as `for_key` but implemented as `for_api`.

Closes denoland#1323
mmastrac pushed a commit that referenced this issue Sep 18, 2023
…pi` (#1324)

`for_global` was documented as `for_key` but implemented as `for_api`.

Closes #1323
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants